You’re doing it wrong #2

 

Welcome to part #2, If you missed #1, go check it out.

As i mentioned in the last posting, This time wp125 is featured again, No, Please dont get me wrong, i’m not just targeting certain plugins here, It’s merely the plugins which I use, which i have to modify  and/or cleanup for whatever reason, I’ve chosen WP125 to be used for this project, so here i am cleaning up some code. Also featured in the 2nd part of this posting, is TDO Mini Forms.

Who has ever seen an error message like this one?

Notice: Undefined index: wp125action in G:\www\nrtt\wp\wp-content\plugins\wp125\adminmenus.php on line 9
Notice: Undefined index: wp125action in G:\www\nrtt\wp\wp-content\plugins\wp125\adminmenus.php on line 13
Notice: Undefined index: wp125action in G:\www\nrtt\wp\wp-content\plugins\wp125\adminmenus.php on line 22
Notice: Undefined index: wp125action in G:\www\nrtt\wp\wp-content\plugins\wp125\adminmenus.php on line 26

I’m willing to bet  a lot of people would’ve seen this one time or another, It comes down to a very very VERY lazy developer in my opinion, Simply because its best programming practice to never actually hit this case..

The code which is causing this:

function wp125_write_managemenu() {
…<snip>…
//Handle deactivations
if ($_GET[‘wp125action’] == “deactivate”) {

Doesnt look too harmful really, now does it? Thats because, By itself, Its not harmful at all other than an annoying message, The harmful part, is where similar code is used, and its merely assumed that certain array items exist, The issue arises that it can make bugs slip by unnoticed..

So, Whats the correct way? Simply check that the Array item exists before comparing it against something else.

The simplest method would be:

if ( isset($_GET['wp125action']) && ($_GET['wp125action'] == "deactivate") {

Or alternatively, If you never want to fire when the array item is empty:

 if ( !empty($_GET['wp125action']) && ($_GET['wp125action'] == "deactivate") {

Now, that wasnt too hard was it? Much cleaner, reduces warnings, and potentially reduces the risk of bugs.

Once again, All changes made are available as a Diff, This diff also includes the changes made in par #1, This has been written as of version 1.3.6.

Now, Onto the second plugin, TDO Mini Forms, This isnt actually a half bad plugin overall,  However, The code can be a bit messy for lack of a better word thanks to the many many many options and defines it uses.

Most of the issues i’ve got with this plugin, boils down to mis-use of constants, for example:

Notice Use of undefined constant TDOMF_OPTION_WIDGET_MAX_HEIGHT - assumed 'TDOMF_OPTION_WIDGET_MAX_HEIGHT' in G:\www\nrtt\wp\wp-content\plugins\tdo-mini-forms\admin\tdomf-options.php on line 604

Upon actually looking through he code, The Define was used in many places, but never actually defined. However, TDOMF_OPTION_WIDGET_MAX_WIDTH, and TDOMF_OPTION_WIDGET_MAX_LENGTH were, But guess what, The latter was never actually used, other than during option creation.. It’s a simple typo really.. But a quick fix never the less.

The main thing that has been bugging me with this plugin however, are these splattered around:

Notice: Use of undefined constant REQUEST_URI - assumed 'REQUEST_URI' in G:\www\nrtt\wp\wp-content\plugins\tdo-mini-forms\admin\tdomf-form-options.php on line 15

It looks like the plugin is expecting some form of register_globals for  the $_SERVER items to be enabled, Well, Do i have news for you… Its not! To many programmers that  may have sounded like the actual problem, But the problem is actually a coding flaw.. (as expected)

if(preg_match('/tdomf_show_form_menu/',$_SERVER[REQUEST_URI])) {

May not see something wrong with that, But, You should. $_SERVER contains an array element called ‘REQUEST_URI’, which is what the author intended to access, But instead, what they have asked for, Is the $_SERVER array element whose name is within the REQUEST_URI definition.. PHP is smart enough to convert that REQUEST_URI into a string, and so the code works as expected, for now.. But it’s still sloppy, Adding 2 apostrophe’s into the mix fixes everything.. Quick and simple really..

if(preg_match(‘/tdomf_show_form_menu/’,$_SERVER[‘REQUEST_URI’])) {

And the final piece of the puzzle for this posting:

Notice: Trying to get property of non-object in G:\www\nrtt\wp\wp-content\plugins\tdo-mini-forms\admin\tdomf-edit-post-panel.php on line 36

Another common type of warning produced by PHP, Very similar to the first Array item above:

function tdomf_edit_post_panel_admin_head() {
   global $post;
   // don't show on new post/page
   if($post->ID > 0) {

Now, This wouldnt be all that bad really.. If it wasnt for this code:

add_action( 'admin_head', 'tdomf_edit_post_panel_admin_head' );

The end result, much like I explained in Post #1, Is that running code designed for a SINGLE page on EVERY page load, is not a good thing to do, eventually you’ll hit a road bump like this one..

While the most appropriate fix for this, would be to simply only hook the function to run on the post edit page, Due to this plugins insisting to be backwards compatible at one stage or another utilising the latest hook names is not always possible, So merely adding an is_object() call in there can silence and fix everything quickly:

function tdomf_edit_post_panel_admin_head() {
 global $post;
 // don't show on new post/page
 if(is_object($post) && $post->ID > 0) {

I should however note, That this plugin includes compat code for WordPress < 2.5, Whilst, It utilises WordPress 2.8 functionalities now. Plugin Authors: Keep an eye on your obsolete code, it increases complexity, and will eventually end up causing a bug. My methodology is to only support the latest WordPress release.. It’s not worth your time developing for users not upgrading their version of WordPress. Yes, You’re going to have people complain the plugin isnt compatible, but in reality, you’re doing them a favour, If they dont upgrade WordPress, they’ll have other bugs.. Your plugin not working is the least of their worries (Or should be) .

Thats it for now, TDO Mini Forms also contains many MANY uses of undefined variables, eg:

Notice: Undefined variable: edit in G:\www\nrtt\wp\wp-content\plugins\tdo-mini-forms\include\tdomf-form.php on line 387

But the plugin is too large for me to want to go in and fix everything,  what has impacted me the most has been fixed, i’ll leave it as that.

Until next time, The changes made to TDO Mini Forms is available as a Diff, as of version 0.13.5. Apologies for the Diff here,I’m having issues with Line endings, Tortoise SVN isn’t respecting its own setting – You’ll have to patch a local copy and set it to ignore line ending changes..

 

EDIT: Fixed typo’s and lack of English. Sorry, I need a new computer, this T key hardly ever works when i want it to..

18 thoughts on “You’re doing it wrong #2”

    1. I’m quite aware of that :)

      To be honest, I’m sick of modifying plugins, and sending diff’s back whenever i want to integrate it with something else.. Its time to just rant in public :)

    1. God.. I wish.. Some auhors i’ve contacted in the past have had excuses such as “..Normal users of my Plugins do not run with WP_DEBUG set to true, therefor, i do not care” others have told me indepth on how to disable error reporting all together.. Others have blamed WordPress for allowing them to code like they did..

      1. That is the stupidest excuse I’ve heard. When Code generates warnings and notices on every page load it slows down your site, and could put your users at risk of worse.

        I may have to take up the “fight” with you and start publicaly patching plugins in this manner.

        1. I admit I don’t code WP plugins with WP_DEBUG neither, I guess that’s a habit from the time when this setting didn’t exist and WP itself was outputting *tons* of warnings with notices turned on (this said, when I start a new project outside of WP, I always turn on debugging)

          I don’t think it slows down the site however. It’s just a message FYI if you elect to display them.

          1. I agree with you there.. For a lot of general users, Its not a problem, as most people run with warnings turned off.

            The only time that it changes, Is when a fellow Programmer or Designer comes along and wants to create something of their own, To turn on debugging and then realise that the components you have chosen make life difficult.. Well thats where it ends for me..

  1. For the first case, I’d say array_key_exists() is the most appropriate function as it tests for key existence.

    Also, this post now just goes into PHP mistakes, rather than WP specifically. I’d rather see some more WP-specific examples in the future.

    1. It’ll vary, I’m intending to focus on things that WordPress PHP Developers do wrong in their plugins, which make the experience for users worse off, If that includes PHP stupidities, then so be it.

      array_key_exists() is good for specific purposes, but isnt exactly what i’d call a general-use function. If you want to determine if an indicy exists, regardless of content, that’ll do, But for more intents, knowing its either a. Existing with a non-null value (isset()) or b. Existing with a non-empty value(empty()) is much more of a common use-case.

      1. Agree. isset is much better to check if an array is set and then if that var is set. I see a lot of plugins coded with is_array() and that might not always work as well since $_GET will be an array, but that index might not exist, resulting in the same error.

  2. This is a good series, but I find it ironic that you have so many typos in your text…. :-/

    It’s “merely”, not “mearly”, for starters….

    I look forward to the next. :-)

    1. Spelling and Grammar are not my thing :) If you knew me well, You’d know i know 2 languages, English, and D’lish.

      That being said.. I should probably run AfterTheDeadline over my posts..

  3. Hi DD32, thanks for the TDO Mini Forms diff. I’m attempting to applying your changes now so they’ll be rolled into the next version. If you posted this on the forums, I’m sorry I missed it (I’m barely keeping up on bug/feature requests).

    I’m not going to offer any excuses for the bad state of the code except to say, I’m writing the plugin for fun and I’m certainly not making any profit on it compared to time invested.

    1. Hi Mark,
      I know exactly where you’re coming from, I don’t make money off the plugins i write, and they just get relegated to the bottom of the to-do pile.

      I hadn’t gotten around to contacting you in any way yet, once again, simply due to it being down my priority list. would’ve happened eventually…

      Nothing against you personally of course, If it wasn’t yours, i can assure you it’d have been the next plugin i picked up..

      Thanks for looking to integrate any of the suggestions though :)

Leave a Reply

Your email address will not be published. Required fields are marked *