It would be nice to keep the getLocationWithParams pattern. The idea is that the click handler can read the current path at the time when it's called, non-reactively. The component doesn't need to be rerendered on every path change and the callback doesn't need to be recomputed.

The problem is that we need to know the registered routes, it's not just about reading the arguments from the url, so IMO, it's better to remove it entirely.

Then we can expose a method on the history = useHistory() object that does the job of useMatch. You can call it imperatively/synchronously at any time, and subscribe to updates. Then the actual useLocation will merely a React binding (with useSyncExternalStore) for this.

One of the follow-up steps will be to somehow abstract away and remove this conditional navigation. When the rewrite rules are enabled, the target URL should be

document.location = `/wp-admin/design/${ postType }/${ record.id }?canvas=edit`;

The history.navigate call

history.navigate( `/${ postType }/${ record.id }?canvas-edit` );

is almost identical except the /wp-admin/design base path. If the "base paths" match, it's an SPA navigation, and a full navigation otherwise.

I think this fix deserves some detailed explanation in a comment. It looks completely magical to me, I don't know what it does, I don't even know what a "hybrid theme" is.

haha, you're right. Basically, site-editor.php's access is forbidden for hybrid/classic themes and only allowed with some very special query args (some very special pages like template parts...). This is done in Core and the only way to disable it if we change urls in Gutenberg is to override the wp_die_handler. (We did this in the past when we introduced the patterns page in classic themes too).

I think in the backport PR of this PR, I'm going to try to remove that blocking condition entirely from Core cause I believe it doesn't really make much sense anymore.

But I'll add a comment.

I see, so we're trying to undo the effect of this code?

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/site-editor.php#L29-L35

What would explain it to me would be:

• mentioning/linking this code explicitly in a comment
• saying how exactly are we going to modify/delete this code when backporting this compat patch to Core

Such information will be also super useful for the person who will actually do the 6.8 backporting.

Is the last comment good enough or you think I should modify it? I'm doing the backporting btw since now it's mandatory to open a PR before being able to merge this PR into Gutenberg :)

Now when I know what's going on I'm personally satisfied 🙂 And it's written down in GitHub comments, a motivated researcher can track it down.

@youknowriad Do you remember why you had to modify this? It looks like the wp_theme_preview query arg could be an empty string or some other falsy value, but the details are not clear. The old code, with its !== undefined check, returns true if the wp_theme_preview query arg is present, with any value.

I remember it partially. I think it has to do with the "active theme" button. Basically when activating a theme, we actually set the wp_theme_preview to empty string (but keep in the url), for the site editor to adapt properly.

There's a reason it's kept in the url instead of undefined though but I don't remember the reason.