WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 21 months ago

#15178 closed defect (bug) (duplicate)

Empty endpoint same as missing endpoint: empty vs isset query_var.

Reported by: 5ubliminal Owned by: johnbillion
Milestone: Priority: normal
Severity: normal Version: 3.0.1
Component: Query Keywords: has-patch dev-feedback
Focuses: Cc:

Description

http://wordpress.org/support/topic/custom-rewrite-approach-with-add_rewrite_endpoint

If I have a /test/ endpoint, it will not show in query_vars unless it has a value. So /test/1 will have query_vars[test]=1 but /test/ will NOT have query_vars[test]=null in query_vars. Empty endpoints should be present with null value in query_vars so they can be tested with isset() for value-less existence.

Hope it make sense. Tested in 3.0.1 and this problem exists.

Attachments (1)

15178.patch (3.8 KB) - added by johnbillion 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 @dd325 years ago

  • Component changed from Permalinks to Query
  • Keywords reporter-feedback added

See also #10710 which may have fixed this already in trunk(3.1)

comment:2 @5ubliminal5 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

That's it! Thanks.

comment:3 @scribu5 years ago

  • Milestone Awaiting Review deleted

comment:4 @johnbillion4 years ago

  • Keywords reporter-feedback removed
  • Resolution duplicate deleted
  • Status changed from closed to reopened

This wasn't in fact fixed in #10710. An endpoint without a value is still indistinguishable from no endpoint. All #10710 done was to explicitly set all query vars to an empty string.

example.com/test/ and example.com/ both give us query_vars[test]='' (an empty string), making them indistinguishable.

Additionally, the inline doc for add_rewrite_endpoint() is misleading as it states:

Add an endpoint, like /trackback/.

which suggests endpoints without values are possible.

Version 0, edited 4 years ago by johnbillion (next)

comment:5 @SergeyBiryukov4 years ago

  • Milestone set to Awaiting Review

comment:6 @johnbillion4 years ago

  • Owner set to johnbillion
  • Status changed from reopened to accepted

comment:7 @johnbillion4 years ago

  • Keywords has-patch dev-feedback added

Here's my take on this. To allow an empty endpoint (ie. an endpoint with no trailing value) we need to set the endpoint's query var to something suitable when the endpoint is present but empty.

In the attached patch I've opted to use string 'true' for empty endpoints. add_rewrite_endpoint() gets an optional third parameter, $allow_empty, which tells WP_Rerite to set the endpoint query var to string 'true' when it's empty. The addition of this parameter maintains backwards compatibilty with anything that's using add_rewrite_endpoint() and not expecting an empty endpoint to have a value.

Inline docs have also been updated.

Feedback welcome.

@johnbillion4 years ago

comment:8 follow-up: @nacin4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

otto42 and I recently conducted a thought experiment on all of this as well. #10710 did actually make this much easier. Previously, the value didn't exist in WP_Query->query_vars at all. Now it does, it's just empty. You can check for isset() against it and it works just fine.

The problem is with WP_Query::get(), which returns '' instead of false when nothing is set.

I'd be willing to modify that to be false. Because it's a query variable, it normally will always return a string, so I doubt the change will hurt any existing code.

The other option is a WP_Query::exists() method, which simply returns the results of isset(). Unfortunately this would not work easily for core query variables, as those are set to empty (but not null) values in fill_query_vars(). At that point we'd probably want to set all string-based QVs to null instead, and then keep a list of array-based QVs to then return the results of empty() on, or something.

I don't think another rewrite rule is necessary. Additionally, it'd end up with 'true' as a string, rather than a boolean. And a string of '1' might conflict with situations where you do have the ability to specify a endpoint values in addition to leaving it blank. Because this was actually fixed in some regard, I'm going to close it again. Feel free to open a new ticket to address the ::get() issue, but at the very least, you now have a workaround.

comment:9 in reply to: ↑ 8 @johnbillion4 years ago

Interesting points nacin. The problem is indeed with WP_Query->get() (and get_query_var()).

The return value of string true in my patch was intentional. It removed the ambiguity with values of a non-empty endpoint (eg. if '1' was used instead of 'true'), and would be consistent with the value of ?my_endpoint=true if rewrites weren't enabled. A moot point though.

+1 for changing WP_Query->get() so it returns boolean false for unset query vars, and/or introducing an exists() method.

Note: See TracTickets for help on using tickets.