checked/selected helper confuses '0' and ''

[Denis-de-Bernardy]

attached patch fixes this, but we need to run some tests in the admin interface to make sure there are no areas where this might interfere with current settings.

[westi]

What test cases would you suggest.

From memory this works the same way the old checked and selected functions did.

[westi]

Moving to 2.9, these functions work in the same way as they did before the refactor.

[Denis-de-Bernardy]

nm. ran a few more tests and it's working fine.

[Denis-de-Bernardy]

silly me, I had my patch applied, no wonder it was working. *grin*.

try:

checked(0, "");

[Denis-de-Bernardy]

verified:

• post visibility
• sticky status
• post comment/ping status
• comment approved status
• link visibility status
• connection type in file.php
• the various stuff under settings
• profile/edit user stuff
• widgets screen without js
• all of the built-in widgets

[Denis-de-Bernardy]

forgot to mention the various menu filters, too.

[azaozz]

The patch seems to fix some instances but may break some too and it still doesn't cover all possibilities.

IMHO this is typical function bloat. Why do we need three functions to deal with something as simple as

if ( get_option('blah') ) echo ' checked="checked"';

They not only make the code harder to read but will need two extra arguments to cover all possible cases (single/double quotes and strict comparison). That's a total of 5 arguments!

Don't see any benefits in using these functions, even in the simple cases where they work well the code is harder to read which is a big minus.

[Denis-de-Bernardy]

Replying to azaozz:

The patch seems to fix some instances but may break some too and it still doesn't cover all possibilities.

which ones aren't covered?

IMHO this is typical function bloat. Why do we need three functions to deal with something as simple (...)
Don't see any benefits in using these functions, even in the simple cases where they work well the code is harder to read which is a big minus.

I wouldn't know. I didn't write them, and only started (2 or 3 years later) to use them. :D

[azaozz]

Think checked() and selected() are from 2.7 (the phpdoc there is wrong) and __checked_selected_helper( $helper, $current, $echo, $type) is new in 2.8...

[Denis-de-Bernardy]

Actually, checked() and selected() have been around for as long as I can remember. :-)

​http://core.trac.wordpress.org/browser/tags/1.5/wp-admin/admin-functions.php

[azaozz]

Heh, the phpdoc there is really wrong. They look a lot better in 1.5 than now. Don't even need two args, one arg would work fine and will make them more compatible (if they only echo the string). Perhaps we need to revert then.

function checked($arg) {
  if ( $arg )
    echo ' checked="checked"';
}

used like:

checked( get_option('blah') );

or

checked( $blah === 0 );

[filosofo]

My copy of 1.2.1 has those functions.

See #9032 for recent changes.

[westi]

Can someone explain clearly what the bug here is.

As far as I can recall the changes preserved the behaviour from 2.7 for this functions and just removed duplicated code.

Looking back at the code change the only regression in behaviour is the change from using double quotes to single quotes for the attribute.

Is this a new bug we are discussing here or an existing bug?

Moving into 2.8.2 for now until we get clarity on what the issue is.

[Denis-de-Bernardy]

@westi. As highlighted in my comment above, the issue is this:

<p>Your Preference</p>
<?php $var = 0; ?>
<select>
<option value="" <?php checked($var, ''); ?>>Site Default</option>
<option value="1" <?php checked($var, 1); ?>>Yes</option>
<option value="0" <?php checked($var, 0); ?>>No</option>
</select>

The change affect nothing in the WP code. But it's still a bug.

[westi]

Replying to Denis-de-Bernardy:

@westi. As highlighted in my comment above, the issue is this:

<p>Your Preference</p>
<?php $var = 0; ?>
<select>
<option value="" <?php checked($var, ''); ?>>Site Default</option>
<option value="1" <?php checked($var, 1); ?>>Yes</option>
<option value="0" <?php checked($var, 0); ?>>No</option>
</select>

The change affect nothing in the WP code. But it's still a bug.

Thanks.

This isn't a new bug just an existing one and I would have thought you should have just used a real value for all three of the choices.

[Denis-de-Bernardy]

per our discussion in IRC

[westi]

(In [11703]) Make checked and selected compare more carefully and update the phpdoc with the correct @since version. Fixes #9862 props Denis-de-Bernardy.

[westi]

BTW - Added Unit Tests for these common call types - ​http://svn.automattic.com/wordpress-tests/wp-testcase/test_admin_includes_template.php

The test cases work without the (string) casts but as the testing was done with them I left them in.

[Denis-de-Bernardy]

@westi: I attached some more test cases, that better highlight the subtle change in behavior.

[Denis-de-Bernardy]

side note on this, the more I think about it, the more I'm feeling that changing the behavior for checked() might be wrong. selected() clearly needs the change imo, checked() is less sure.

[westi]

Replying to Denis-de-Bernardy:

@westi: I attached some more test cases, that better highlight the subtle change in behavior.

Thank you. I've committed them to the wordpress-tests repo

Replying to Denis-de-Bernardy:

side note on this, the more I think about it, the more I'm feeling that changing the behavior for checked() might be wrong. selected() clearly needs the change imo, checked() is less sure.

I think that they should have consistent behaviour between the two functions so if we improve one we improve the other and they have always had consistent behaviour and it makes it easier to use them.