WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#31723 closed defect (bug) (wontfix)

is_page( false ) === true ?

Reported by: Compute Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: needs-unit-tests 2nd-opinion
Focuses: Cc:

Description

https://developer.wordpress.org/reference/classes/wp_query/is_page/

Is the query for an existing single page?

But this really does not make sense to me:

if ( empty( $page ) )
    return true;

Is there a reason that is_page() will return true on empty, false, 0 values?

Attachments (2)

31723.diff (1.5 KB) - added by jipmoors 3 years ago.
to null change on default value
31723.1.diff (15.6 KB) - added by jipmoors 3 years ago.
applied to all functions + added unit tests for invalid data

Download all attachments as: .zip

Change History (15)

#1 @SergeyBiryukov
3 years ago

Introduced in [1728].

#2 follow-up: @valendesigns
3 years ago

Unfortunately, if we were to make that simple change and return false then 36 tests fail. I do agree that it seems wrong that it would return true in these situations, but doing anything about it would require a lot of refactoring of tests and could have wide sweeping backwards compatibility implications. I'm inclined to close as wontfix because of it.

#3 in reply to: ↑ 2 @jipmoors
3 years ago

Replying to valendesigns:

Unfortunately, if we were to make that simple change and return false then 36 tests fail. I do agree that it seems wrong that it would return true in these situations, but doing anything about it would require a lot of refactoring of tests and could have wide sweeping backwards compatibility implications. I'm inclined to close as wontfix because of it.

I disagree, a simple modification on the default value would solve this problem. The way it is set up now is pretty ugly and can simply be improved upon by setting the default to null. This will keep all the tests passing and prevents bad data from being entered.

@jipmoors
3 years ago

to null change on default value

#4 @jipmoors
3 years ago

  • Keywords has-patch added

#5 @valendesigns
3 years ago

That's an interesting approach. I didn't think about setting it to null. Though it still seems odd that a null value would return true.

#6 @jipmoors
3 years ago

No it's quite logical; null means that it will respect the $this->is_page value. The is_null check will only be ran when $this->is_page is not false.

It would be more readable if it would say return (bool) $this->is_page but that is basically what it does without the use of the variable.

If we don't have some custom object, list or ID to check against, the global query is used which already determined if it is a page.

#7 @jipmoors
3 years ago

  • Keywords has-patch removed

Actually. Looking through the file, some other functions could use this treatment aswel. Might be good to check if there was a decision not to use this approach or some ticket about this is still opened.

In each case the patch is incomplete.

#8 @boonebgorges
3 years ago

  • Keywords needs-unit-tests added

What exactly is the bug here, aside from the fact that it seems weird? Did an actual situation come up where you were passing false or 0 to is_page()?

See #24674 [31458] for a related issue.

#9 @Compute
3 years ago

I have an option that filters the query:

if ( is_page( get_option( 'special_filter' ) ) {
    $query->set( 'special_filter', 1 );
}

Above works if the option has been set (as the ID might be 54).

But if the option has not been set it returns true on all pages. The fix is really to check for the option before passing it into the is_page() check:

$option = get_option( 'special_filter' );

if ( ! $option ) {
    return $query;
}

if ( is_page( $option ) {
    $query->set( 'special_filter', 1 );
}

To me it just seems wierd that is_page( false ) is true. If this is really working as intended then the description should be updated.

I truely understand if it's a wontfix, I was just wondering WHY this makes any sense.

#10 @jipmoors
3 years ago

  • Keywords dev-feedback added

While testing code: returning true or false on empty(ish) input mostly doesn't matter; though one of the two should be an unwanted return value.

The question is: do we want to invalidate unusable values or assume 'current' item variables apply

Logic dictates that we invalidate anything other than an empty call, because the default has been overridden. If this is applied to all is_* functions in query.php only one test breaks: entering an empty array for is_tag is_tag(array()) which expects to be true.

I can understand why that test has been applied; and it should be removed or applied to all other functions aswel. There are no comments on the test and situations this is created for.

Feedback requested on validity of this test

Writing the needed tests will be possible when the goal/desire/expectations are clearer.

Suggested 'template' for functions with one parameter:

function is_param($param = null) {
  if ( is_null( $param ) || ! $this->is_param )
	return (bool) $this->is_param;

  $param = (array) $param;

  if ( empty( $param ) )
	return true;

  ... code ...

  return false
}

Changes:

  • Changed default value of parameter from '' to null.
  • Moved the casting to array up in the function.
  • Returning true on empty (array).

This way an empty array can be entered for default behaviour, anything else will be made into an array value so page titles can have '0' and be found.

All checks inside the function return 'true' except the first and last ones. Which provides much more readable code and debugging.

If we want to invalidate the empty array the only thing to do is swap the array casting and empty check.

I have a patch with this applied to all functions and they pass all current tests.

#11 @jipmoors
3 years ago

  • Keywords 2nd-opinion added; dev-feedback removed

@jipmoors
3 years ago

applied to all functions + added unit tests for invalid data

#12 @boonebgorges
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I truely understand if it's a wontfix, I was just wondering WHY this makes any sense.

In a nutshell: is_page() (and is_author(), etc) is intended to be used in two different ways. One is to pass a specific ID or slug: is_page( 'foo' ), is_page( 123 ). The other is the more general check "is this a page at all?", where no param is passed: is_page(). So the logic of these functions goes something like this:

If we were designing the system from scratch, something like jipmoors's suggestion would be good. Or even something more explicit, using argument overloading:

public function is_page() {
    if ( ! func_num_args() ) {
        return $this->is_page;
    }

    $page = func_get_arg( 0 );
    // rest of the function
}

31723.1.diff fixes the behavior for is_page( 0 ) and is_page( false ) and is_page( '' ), but not for is_page( null ). I suppose there's an argument for privileging '' and false: get_metadata() will generally return an empty string when the requested value is not found, and get_option() generally returns false, which means that the bug'll be fixed for cases where a falsey value is stored in one of these two places. My func_num_args() trick is a broader fix, though I know that in the past, function argument overloading has been frowned upon (harder to read and document).

In any of these cases, backward compatibility is a concern. It's very likely that plugins are counting on is_page( 0 ) to return the same as is_page(). It's a hard thing to verify, though - I'm not sure how to search for it.

I'm leaning toward wontfix. As Compute notes, there is an easy workaround, and you might argue that plugin authors ought to be doing this kind of validation anyway. If anyone has a powerful practical argument that wontfix is the wrong way forward here, please feel free to reopen with details.

Last edited 3 years ago by boonebgorges (previous) (diff)

#13 @SergeyBiryukov
3 years ago

#32267 was marked as a duplicate.

Note: See TracTickets for help on using tickets.