WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 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 6 years ago.
test_admin_includes_template.php.diff (2.6 KB) - added by Denis-de-Bernardy 6 years ago.

Download all attachments as: .zip

Change History (25)

@Denis-de-Bernardy6 years ago

comment:1 @westi6 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.

comment:2 @westi6 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.

comment:3 @Denis-de-Bernardy6 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.

comment:4 @Denis-de-Bernardy6 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, "");

comment:5 @Denis-de-Bernardy6 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

comment:6 @Denis-de-Bernardy6 years ago

forgot to mention the various menu filters, too.

comment:7 follow-up: @azaozz6 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.

comment:8 in reply to: ↑ 7 @Denis-de-Bernardy6 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

comment:9 @azaozz6 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...

comment:10 @Denis-de-Bernardy6 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

comment:11 @azaozz6 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 );

comment:12 @filosofo6 years ago

My copy of 1.2.1 has those functions.

See #9032 for recent changes.

comment:13 @westi6 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.

comment:14 follow-up: @Denis-de-Bernardy6 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.

comment:15 in reply to: ↑ 14 @westi6 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.

comment:16 @Denis-de-Bernardy6 years ago

  • Milestone changed from 2.8.2 to 2.9

per our discussion in IRC

comment:17 @westi6 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.

comment:18 @westi6 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.

comment:19 follow-up: @Denis-de-Bernardy6 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.

comment:20 @Denis-de-Bernardy6 years ago

  • Status changed from reopened to assigned

comment:21 @Denis-de-Bernardy6 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.

comment:22 in reply to: ↑ 19 @westi6 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.

comment:23 @westi6 years ago

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