WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#9862 closed defect (bug) (fixed)

checked/selected helper confuses '0' and ''

Reported by: Denis-de-Bernardy Owned by: westi
Milestone: 2.9 Priority: low
Severity: normal Version: 2.8
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (2)

9862.diff (501 bytes) - added by Denis-de-Bernardy 8 years ago.
test_admin_includes_template.php.diff (2.6 KB) - added by Denis-de-Bernardy 7 years ago.

Download all attachments as: .zip

Change History (25)

#1 @westi
8 years ago

  • Owner set to westi
  • Priority changed from normal to low
  • Status changed from new to accepted

What test cases would you suggest.

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

#2 @westi
8 years ago

  • Milestone changed from 2.8 to 2.9

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

#3 @Denis-de-Bernardy
7 years ago

  • Milestone 2.9 deleted
  • Resolution set to invalid
  • Status changed from accepted to closed

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

#4 @Denis-de-Bernardy
7 years ago

  • Milestone set to 2.8.1
  • Resolution invalid deleted
  • Status changed from closed to reopened

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

try:

checked(0, "");

#5 @Denis-de-Bernardy
7 years ago

  • Keywords commit added; needs-testing removed

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

#6 @Denis-de-Bernardy
7 years ago

forgot to mention the various menu filters, too.

#7 follow-up: @azaozz
7 years ago

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.

#8 in reply to: ↑ 7 @Denis-de-Bernardy
7 years ago

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

#9 @azaozz
7 years ago

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...

#10 @Denis-de-Bernardy
7 years ago

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

#11 @azaozz
7 years ago

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 );

#12 @filosofo
7 years ago

My copy of 1.2.1 has those functions.

See #9032 for recent changes.

#13 @westi
7 years ago

  • Keywords commit removed
  • Milestone changed from 2.8.1 to 2.8.2

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.

#14 follow-up: @Denis-de-Bernardy
7 years ago

  • Keywords commit added

@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.

#15 in reply to: ↑ 14 @westi
7 years ago

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.

#16 @Denis-de-Bernardy
7 years ago

  • Milestone changed from 2.8.2 to 2.9

per our discussion in IRC

#17 @westi
7 years ago

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

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

#18 @westi
7 years ago

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.

#19 follow-up: @Denis-de-Bernardy
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#20 @Denis-de-Bernardy
7 years ago

  • Status changed from reopened to assigned

#21 @Denis-de-Bernardy
7 years ago

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.

#22 in reply to: ↑ 19 @westi
7 years ago

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.

#23 @westi
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.