[REST] Restore the missing double slash in the ID received by /templates

[adamziel]

Description

Solves https://core.trac.wordpress.org/ticket/54507

When the permalink structure is set to /index.php/%year%/%monthnum%/%day%/%postname%/, requests to /wp/v2/templates/twentytwentytwo//home fail, breaking the full site editor. The problem is double slash in the request path. WordPress resolves $request['id'] as twentytwentytwo/home instead of twentytwentytwo//home. The endpoint has already been shipped in WordPress 5.8, so changing the URLs structure isn't really an option. This PR proposes a way of processing the received ID so the slashes are always correct regardless of how they came in.

Test plan

1. Go to WP Admin → Settings → Permalinks.
2. Select Custom Structure and enter /index.php/%year%/%monthnum%/%day%/%postname%/ into the text field.
3. Go to WP Admin → Appearance → Editor.
4. Confirm the editor loads as expected with no WSOD in sight.

If this PR gets merged, it needs to also be backported into WP Beta.

[TimothyBJacobs]

Will this work for subdirectory themes like theme-experiments/tt1-blocks?

[youknowriad]

The double slash is there for themes inside subfolders, the theme names for this kind of themes contain a "/" so that's why we needed the double slash. So you need to test with these themes as well.

[adamziel]

Good points @youknowriad and @TimothyBJacobs. I think there are two options here:

1. Try different cut-off points, e.g. a//b//c//d, a/b//c//d, a/b/c//d – it's O(n) so not too terrible. The loop would take each slash and try to double all the slashes on the right.
2. Extract the relevant fragment from $_SERVER[ 'REQUEST_URI' ] and use that as a fallback.

What do you think?

[TimothyBJacobs]

Does each check require a database query?

[TimothyBJacobs]

And to clarify, the double slash is between the theme slug and the template slug. So you'd only ever have one double slash, and subdirectory themes aren't allowed at an arbitrary depth, only one level.

[adamziel]

It does @TimothyBJacobs, with the REQUEST_URI way there would be less database queries.

[TimothyBJacobs]

Yeah I think we could look at REQUEST_URI here. Though I think it'd be ideal if WP Rewrite would handle making sure the path is right in the first place. Not sure how feasible that is.

[Mamaduka]

@adamziel, what if we try to normalize ID before making a DB call?

Something like: https://3v4l.org/JU4LI

$bad_id = 'twentytwentytwo/index';
$good_id = 'twentytwentytwo//index';
$theme = 'twentytwentytwo';

var_dump( preg_replace( "|$theme/+|", "$theme//", $bad_id ) );
var_dump( preg_replace( "|$theme/+|", "$theme//", $good_id ) );

I think %year%/%monthnum%/%day%/%postname%/ might be a popular permalink structure, and we might end up doing WP_Queries for each request.

P.S. Sorry about the bad regex.

[adamziel]

@TimothyBJacobs I think the culprit is what @noisysocks mentioned in the original ticket:

This is because, when the "almost pretty" permalink structure, WordPress will use $_SERVER['PATH_INFO'] as the route. PATH_INFO will collapse double slashes.

We would have to infer that information from REQUEST_URI. I fear there will be a lot of tricky corner cases, but it's worth a shot.

what if we try to normalize ID before making a DB call?

@Mamaduka I'd love that, but I don't think we can just assume that $theme = get_stylesheet(). Alternatively, we could make REQUEST_URI the default and use $request['id'] as a fallback (or just ignore it).

[TimothyBJacobs]

Alternatively, we could make REQUEST_URI the default and use $request['id'] as a fallback (or just ignore it).

We're going to need some additional checks here that the REQUEST_URI is sane according to the WP_REST_Request object I think. Because the REST server can process a request object without it coming thru WP_REST_Server::serve_request.

[adamziel]

I went for a different, simpler approach. Given that template ID is generated like $theme//$slug and only $theme can contain slashes and $slug cannot, we only have to try doubling the last slash in $request['id']. It is much simpler than parsing REQUEST_URI.

Actually we could even sanitize it so that the last slash is always a double one – it is not optional to have it there.

[adamziel]

I think this is in a pretty good place. The only thing that is missing is a test with template name like theme-experiments/tt1-blocks//index. I'm not sure how to add another theme to the test environment, though. I don't think we need to block this PR on that, let's make it a follow up.

[adamziel]

I went for a unit test instead. I think this one is ready for feedback now. cc @mcsf @spacedmonkey @TimothyBJacobs @Mamaduka @draganescu @getdave

[mcsf]

Given that template ID is generated like $theme//$slug and only $theme can contain slashes and $slug cannot, we only have to try doubling the last slash […]

Oh, that changes everything. I thought part of the difficulty stemmed from there being no tighter rules in how users can name slugs, particularly around possible slashes.

New approach renders my feedback no longer applicable

[adamziel]

Let's confirm that with @youknowriad – from my explorations it seems like the template $slug adheres to the same rules as all other slugs in WordPress.

[Mamaduka]

template $slug adheres to the same rules as all other slugs in WordPress.

That's correct. We just combine $theme//$post->slug for template IDs. So slug will adhere to rules of sanitize_title.

[draganescu]

I think this ended up as a cleaner fix compared to previous explorations. We first default to what just works and only branch to a special case if we need to. Also the _sanitize_template_id is cleaner and easier to follow.

@TimothyBJacobs do you have any further comments?

Also I see the testing instructions is for seeing the WSOD is fixed, but are there any other cases in themes in subfolders than could be affected?

[draganescu]

LGTM with the caveat that I don't know if anything else could be broken by autodoubling we do here, other than fixing our problem. However the resulting changeset is quite lean so if it does break things it's easy to track.

[adamziel]

Also I see the testing instructions is for seeing the WSOD is fixed, but are there any other cases in themes in subfolders than could be affected?

I don't think E2E covers having a theme in a subdirectory, it would be nice to add that (as I mentioned above). Otherwise, I think all the bases are covered. Plus reverting this one should be easy as you said.