WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#35115 closed defect (bug) (fixed)

404 error when URL includes title=...

Reported by: Fuse99 Owned by: pento
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Query Keywords: needs-unit-tests
Focuses: Cc:

Description

Our web site is broken since upgrading to WordPress v4.4. We have several pages where we pass parameters in the URL. Since 4.4, any URL which includes "title=<some_text>", where <some_text> is any text at all, results in a 404 error.

Our permalinks are of the post name format ("/%postname%/"). I have not tested this with any other permalink formats, as changing permalinks would also break our site.

To reproduce, simply add "?title=test" to any page URL, at least while using post name permalinks.

Attachments (2)

35115.patch (2.2 KB) - added by tyxla 3 years ago.
Make title a private query var instead of a public one.
35115.tests.diff (1.3 KB) - added by johnbillion 3 years ago.

Download all attachments as: .zip

Change History (25)

#1 @pento
3 years ago

  • Component changed from General to Query
  • Milestone changed from Awaiting Review to 4.4.1

Hi @Fuse99!

Thank you for the bug report. I've confirmed that this works in 4.3, and is broken in 4.4, so we'll work on getting this fixed up for 4.4.1.

#2 @pento
3 years ago

@wonderboymusic: This is caused by [33706], which is clobbering anything else using title as a URL parameter. Any thoughts on how to keep the title search, while allowing folks to keep using title in their own code?

#3 @dd32
3 years ago

I'm not sure why the title param is a public query var - is it really designed to be used in URLs, or should it be limited to code WP_Querys? (edit: For the purposes of the REST API, maybe having it public is needed, otherwise you might not be able to use it from there..)

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

#4 @strangerstudios
3 years ago

+1

We have customers with forms passing title as a parameter that are breaking because of this. Our example code for adding fields to PMPro checkout pages uses title as the name/key. I'm thinking we might need to add prefixes to those parameters to avoid future conflicts.

FYI here is code I'm using on some sites to disable the title query var. This might be overkill, but it works in a pinch.

https://gist.github.com/strangerstudios/856e900680330ee503ed

#5 @strangerstudios
3 years ago

As a follow up, we already avoid the other public query vars. I don't know if it makes sense (or is even that easy or possible) to have "title" available for queries in code but not by appending ?title= to a URL as @dd32 is suggesting. If there is and that makes sense, I'm game, but IMO, since 4.4 "title" is sacred and we need to find other parameters to use.

I can think of some crazy ways to allow parameter sharing, but they feel really icky to me.

#6 @pento
3 years ago

I agree - unless there's a need I'm not aware of (/?title=Hello%20World is cute, but hardly necessary), title shouldn't be a public query var.

#7 @dennis_f
3 years ago

Just wanted to add - this also happens with POST requests, I have a form (with POST method) on one of the pages on my site that includes a "title" input and it always returns a 404 page after I submit the form.

#8 @peterwilsoncc
3 years ago

  • Keywords needs-patch added

@tyxla
3 years ago

Make title a private query var instead of a public one.

#9 @tyxla
3 years ago

  • Keywords has-patch added; needs-patch removed

#10 @Fuse99
3 years ago

For the benefit of those of us who are not familiar with the code or the context of the variables, could you please clarify that this means the user can continue to use "title=" in the URL when 4.4.1 comes out? If so, thank you very much for the speed with which this has been handled.

#11 @tyxla
3 years ago

@Fuse99: Yes, this change will allow using the title parameter without consequences in the URLs. This is the main goal IMHO, as this was working properly before 4.4.

The idea is to still allow querying posts by title (as introduced in [33706]), but without the option to query them directly through the URLs (which was causing 404s in GET and POST requests that contain title parameter).

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

#12 @pento
3 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 36034:

Query: Remove title from the public query vars list.

[33706] added title as a public query var, but there's not really a practical need for this, and it interferes with any plugin that uses title as a query var for itself.

Props tyxla.

Fixes #35115 for trunk.

#13 @pento
3 years ago

  • Keywords fixed-major added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#14 @johnbillion
3 years ago

  • Keywords has-unit-tests added

It might be a good idea to add a unit test for public query vars, so whenever a new public query var is introduced it at least needs the test updating, making it a more explicit action in that regard.

35115.tests.diff adds such a test.

#15 @pento
3 years ago

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

In 36035:

Query: Remove title from the public query vars list.

[33706] added title as a public query var, but there's not really a practical need for this, and it interferes with any plugin that uses title as a query var for itself.

Merge of [36034] to the 4.4 branch.

Props tyxla.

Fixes #35115.

#16 @pento
3 years ago

  • Keywords commit added; fixed-major removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

This ticket was mentioned in Slack in #core by pento. View the logs.


3 years ago

#18 @johnbillion
3 years ago

In 36045:

Query: Introduce a unit test which will fail when new public query vars are introduced without also updating the test. This adds an extra layer of explicitness to introducing public query vars in order to avoid introducing unintentional clashes with URL query vars that are already in use.

See #35115

#19 @johnbillion
3 years ago

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

In 36046:

Query: Introduce a unit test which will fail when new public query vars are introduced without also updating the test. This adds an extra layer of explicitness to introducing public query vars in order to avoid introducing unintentional clashes with URL query vars that are already in use.

Merges [36045] to the 4.4 branch.

Fixes #35115

#20 @johnbillion
3 years ago

  • Keywords needs-unit-tests added; has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Tests are failing :/

#21 @johnbillion
3 years ago

In 36048:

Query: Re-initialise any dynamically-added public query vars before running the public query vars test.

See #35115

#22 @johnbillion
3 years ago

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

In 36051:

Query: Re-initialise any dynamically-added public query vars before running the public query vars test.

Merges [36048] to the 4.4 branch.

Fixes #35115

#23 @johnbillion
3 years ago

In 36052:

Tests: Correct the public query vars test for the 4.4 branch.

See #35115

Note: See TracTickets for help on using tickets.