#4275 closed defect (bug) (fixed)
PHP Exec Widgets repeat in WP 2.2 widget implementation
Reported by: |
|
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)
Change History (21)
#2
@
18 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
@
18 years ago
Looks like $number isn't being passed correctly. I swear, every widget I look at does things differently.
#4
follow-up:
↓ 12
@
18 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
@
18 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.
#11
@
18 years ago
Just for grins, attached is a diff that updates execphp to use the new widgets registration API.
#12
in reply to:
↑ 4
@
18 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
@
18 years ago
Otto42, that will work. It will ensure compatibility both with 2.2 and with the plugin.
#14
@
18 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:
↓ 16
↓ 17
@
18 years ago
Your call on whether you want it integrated or not, but let me offer my opinion:
- 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.
- 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.
- 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
@
18 years ago
Replying to Otto42:
- 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
@
18 years ago
Replying to Otto42:
- 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.
- 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
@
18 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
@
18 years ago
Well, more like:
<div class="textwidget"> <?php echo ($codechecked) ? eval('?>' . $text ) : $text; ?> </div>
But I digress. :)
#20
@
18 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>
This one?
http://ottodestruct.com/wpstuff/execphp.zip