Make WordPress Core

Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#4275 closed defect (bug) (fixed)

PHP Exec Widgets repeat in WP 2.2 widget implementation

Reported by: technosailor's profile technosailor Owned by:
Milestone: 2.2.1 Priority: high
Severity: normal Version: 2.2
Component: Administration Keywords: widgets needs-patch
Focuses: Cc:

Description

As widgets should create a completely BW-compat version of the widgets plugin, it is an unnecessary burden on the community to expect common third party widgets such as the PHP Exec widget to behave differently.

Currently, take a fresh install of WP 2.2, activate PHP Exec widget plugin, set the number of PHP widget to 3. Drag all three PHP Code widgets onto a sidebar and set each one to use different bits of PHP code. I used:

<?php
echo'<pre>';
print_r(array('apple','orange'));
echo'</pre>';
?>
<?php
echo'<pre>';
print_r(array('pineapple','mango'));
echo'</pre>';
?>
<?php
echo'<pre>';
print_r(array('grape','cherry'));
echo'</pre>';
?>

When I go and look at the result on the blog, there are three instances of:

Array
(
    [0] => apple
    [1] => orange
)

Bad news.

Attachments (1)

execphp_new_api.diff (550 bytes) - added by ryan 17 years ago.
execphp using the new registration API

Download all attachments as: .zip

Change History (21)

#2 @technosailor
17 years ago

Si Senor. I fiddled with stuff for awhile. I'm sure the widget will need to be updated by Otto - but it sure would be nice if we could find a way to make it work.

#3 @ryan
17 years ago

Looks like $number isn't being passed correctly. I swear, every widget I look at does things differently.

#4 follow-up: @ryan
17 years ago

Oh, wow, the plugin version of widgets had a strange "feature" where classname was included as part of the params to pass to the widget due to slicing func_get_args at 2 instead of 3. This allowed widgets to specify things such as $number in the classname argument. Hard to blame them for doing this since the default text widget did the same thing.

#5 @ryan
17 years ago

A quick fix is to change this:

register_sidebar_widget($name, $i <= $number ? 'widget_execphp' : /* unregister */ '', $i);

to this:

register_sidebar_widget($name, $i <= $number ? 'widget_execphp' : /* unregister */ '', '', $i);

in the plugin. Notice the extra empty argument before $i.

#6 @ryan
17 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [5484]) Restore crack-headed arg passing to register_sidebar_widget(). fixes #4275

#7 @ryan
17 years ago

(In [5485]) Restore crack-headed arg passing to register_sidebar_widget(). fixes #4275 for 2.3

#8 @ryan
17 years ago

Forget the quick fix. widgets.php "fixed"

#9 @technosailor
17 years ago

And the crowd goes wild. Thanks Ryan.

#10 @foolswisdom
17 years ago

  • Version set to 2.2

@ryan
17 years ago

execphp using the new registration API

#11 @ryan
17 years ago

Just for grins, attached is a diff that updates execphp to use the new widgets registration API.

#12 in reply to: ↑ 4 @Otto42
17 years ago

Replying to ryan:

Oh, wow, the plugin version of widgets had a strange "feature" where classname was included as part of the params to pass to the widget due to slicing func_get_args at 2 instead of 3. This allowed widgets to specify things such as $number in the classname argument. Hard to blame them for doing this since the default text widget did the same thing.

Yes. In fact, the ExecPHP widget is an almost exact duplicate of the original text widget, with the addition of some minor code to do eval()'s on php in there (and renaming it and such). Other than that, it's basically the same.

Would it be possible to do something like this to make it compatible both ways?

if (function_exists('wp_register_sidebar_widget'))
{
$id = "php-code-$i";
wp_register_sidebar_widget($id, $name, $i <= $number ? 'widget_execphp' : /* unregister */ '', '', $i);
wp_register_widget_control($id, $name, $i <= $number ? 'widget_execphp_control' : /* unregister */ '', 'width=460&height=350', $i);
} 
else
{
register_sidebar_widget($name, $i <= $number ? 'widget_execphp' : /* unregister */ '', $i);
register_widget_control($name, $i <= $number ? 'widget_execphp_control' : /* unregister */ '', 460, 350, $i);
}

Or is there more to it than that?

If this would work, I'll try to throw that fix into the main ExecPHP code tonight for everybody to grab.

#13 @ryan
17 years ago

Otto42, that will work. It will ensure compatibility both with 2.2 and with the plugin.

#14 @technosailor
17 years ago

Is there a compelling reason why we can't just integrate the plugin into the core? It's very often used, in my experience, and seems like a natural candidate for inclusion.

#15 follow-ups: @Otto42
17 years ago

Your call on whether you want it integrated or not, but let me offer my opinion:

  1. I made it as a plugin to solve my migration problems. It made it extremely easy to migrate my existing sidebar. However, in the long run, I stopped using it as widgets became available to do exactly what I wanted. Now, if I had not made the ExecPHP widget first, somebody else would have. However, on the whole, I think it's bad as it is very simple to use and possibly causes some plugin authors to not bother writing a widget for their sidebar plugins.
  1. It's potentially a security risk for multi-user blogs. Maybe. Some roles/capabilities need to be examined to be sure. I didn't bother adding any extra security layers to it, and don't know if they are needed.
  1. Instead of making a separate widget for it, I suggest adding a checkbox to the Text widget config screen that will turn on/off the execution of PHP code found in the text box. No need for two widgets where one will do.

#16 in reply to: ↑ 15 @foolswisdom
17 years ago

Replying to Otto42:

  1. Instead of making a separate widget for it, I suggest adding a checkbox to the Text widget config screen that will turn on/off the execution of PHP code found in the text box. No need for two widgets where one will do.

I would be opposed to that experience. "Text Widget" is already a misnomer. Rename it the "Code Widget" ;-)

#17 in reply to: ↑ 15 @technosailor
17 years ago

Replying to Otto42:

  1. It's potentially a security risk for multi-user blogs. Maybe. Some roles/capabilities need to be examined to be sure. I didn't bother adding any extra security layers to it, and don't know if they are needed.

Allow me to be the devil's advocate and make a semantic argument. If a user is an administrator, they can modify widgets. If they are not they can't. If they are an administrator then they should have access to all administrative functions. If they shouldn't have access to all administrative functions, then they should be an Editor. So it comes down to a management decision for the blog owner and thus outside of the auspices of the development of WordPress.

  1. Instead of making a separate widget for it, I suggest adding a checkbox to the Text widget config screen that will turn on/off the execution of PHP code found in the text box. No need for two widgets where one will do.

I would agree with this, and I would also agree with foolswisdom's nomenclature argument.

#18 @Otto42
17 years ago

I was unaware of who had access to alter widgets, so I didn't know if it was a security issue or not. Obviously the admin can change/execute anything they want.

If you rename it to "code widget" or whatever, then would you always want it to execute php code as well? Because that's really, really easy.

Just change this:

<div class="textwidget"><?php echo $text; ?></div>

to this:

<div class="textwidget"><?php eval('?>'.$text); ?></div>

Done and done. Okay, you change all the names and such to make it "code widget" as well, but this is the only change of substance. It's what makes all the text get run as PHP.

#19 @technosailor
17 years ago

Well, more like:

<div class="textwidget">
<?php
echo ($codechecked) ? eval('?>' . $text ) : $text;
?>
</div>

But I digress. :)

#20 @Otto42
17 years ago

Bad use of the trinary. eval() returns a null unless there's a return statement in the eval'd code. The eval() itself will cause the output of the non-php code.

Anyway, I really was responded to fools' statement where he seemingly disliked the idea of a checkbox. If you want a checkbox, the right way is:

<div class="textwidget">
<?php
if ($codechecked) eval('?>' . $text );
else echo $text;
?>
</div>
Note: See TracTickets for help on using tickets.