Site Editor: Use path based routing instead of query args and site-editor.php routes #67199
Conversation
|
Size Change: +3.12 kB (+0.17%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
try/permalinks-in-site-editor
branch
from
d1052a3
to
b0c11e8
Compare
youknowriad
changed the title
youknowriad
force-pushed
the
try/permalinks-in-site-editor
branch
from
bc7da78
to
eca0126
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
added
[Type] Code Quality
youknowriad
force-pushed
the
try/permalinks-in-site-editor
branch
from
eca0126
to
f84d4ac
Compare
youknowriad
requested review from
oandregal,
jsnajdr,
tyxla and
jorgefilipecosta
| addQueryArgs( path, { | ||
| layout: newView.type, | ||
| } ) | ||
| ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
youknowriad
force-pushed
the
try/permalinks-in-site-editor
branch
from
3754c93
to
d03a835
Compare
| function gutenberg_remove_query_args( $args ) { | ||
| $query_string = $_SERVER['QUERY_STRING']; | ||
| $query = wp_parse_args( $query_string ); | ||
| foreach ( $args as $arg_name ) { | ||
| unset( $query[ $arg_name ] ); | ||
| } | ||
| return $query; | ||
| } |
There was a problem hiding this comment.
Can't we just use remove_query_arg? It supports multiple query args FWIW.
There was a problem hiding this comment.
While this would work right now. It doesn't work if we switch our proposal to "pretty permalinks" like the PR started because we'll be changing the path in these situations. I'm hesitant to do it because of that. And that code in particular is really not meant to be nice looking, it's just a migration code.
There was a problem hiding this comment.
Got it 👍 but 2 things to note here:
- Pretty permalinks don't really work in the admin just yet
- If pretty permalinks are made to work in the admin, I'd expect them to work the same way as on the frontend, meaning, if you get an ugly permalink URL, WP will use the canonical redirect to redirect it to the pretty permalink automatically, meaning, it will still work in the end.
There was a problem hiding this comment.
Pretty permalinks don't really work in the admin just yet
They actually work, You can revert this PR to this commit 7cbd26946b1bea2371f08293ab53f6647e11564e and explore it.
The reason I decided to not do it is that not all of WP installs support mod_rewrite even though it's mentioned in the WP requirements.
There was a problem hiding this comment.
Interesting 👍 Thanks for mentioning that!
This likely means we either:
- Need to always use the ugly ones (like many plugins do with the REST API for example)
- Have a wrapper that "knows" if pretty permalinks are enabled, and rewrite URLs to it, just like
get_permalink()or any other canonical link function in WP does it.
There was a problem hiding this comment.
Anyway, this is a diversion from the original comment. If you just want to remove the query args, you can always do it with simple means like remove_query_arg( array_keys( $_GET ) ) or something similar - no need for 10 LoC IMHO.
There was a problem hiding this comment.
The problem is not in the removal of the query args is In the return value. I prefer the current code because it maps properly the way frontend work. A site editor url being a "path" + "query" and the special "p" argument in the query is just an implementation logic because we don't have pretty permalinks. Using remove_query_arg means we updated our code to something like
add_query_arg( array('p' => '/navigation' ), remove_query_arg( array( 'postType' ) ) )
I agree it's shorter but we lose the "site editor url abstraction (path + query). Maybe it's ok I don't know.
There was a problem hiding this comment.
About the permalinks:
- Gutenberg can safely call
add_rewrite_rulewithout any checks whethermod_rewriteis enabled etc. Core does that all the time, like when registering post types or taxonomies or setting up REST API. And so do plugins like WooCommerce - a common way to detect whether permalinks are enabled is with
get_option( 'permalink_structure' ). Core functions likeget_trackback_url,get_post_embed_urlorget_rest_urldo this check and return a pretty or ugly URL depending on the result.gutenberg_get_site_editor_urlcould work the same way - one thing that is special about Site Editor URL is that they don't redirect to the main
index.phpfile, but to something else, namelywp-admin/site-editor.php. TheWP_Rewriteclass will think that this is an external redirect, i.e., that the rule redirects you outside WordPress. Not sure what are the consequences. But we might want to add a new rule category ($this->admin_rules) toWP_Rewrite.
| } | ||
|
|
||
| function gutenberg_get_site_editor_url( $path = '', $query = array() ) { | ||
| $query_string = build_query( array_merge( $query, $path ? array( 'p' => $path ) : array() ) ); |
There was a problem hiding this comment.
build_query won't urlencode the query string values by default. Could that be an issue here?
| } | ||
|
|
||
| function gutenberg_get_site_editor_url( $path = '', $query = array() ) { | ||
| $query_string = build_query( array_merge( $query, $path ? array( 'p' => $path ) : array() ) ); |
There was a problem hiding this comment.
Also, it could make sense to not call build_query at all if there are no args to build it from.
| $path ? array( | ||
| 'page' => 'gutenberg-posts-dashboard', | ||
| 'p' => $path, | ||
| ) : array( 'page' => 'gutenberg-posts-dashboard' ) |
There was a problem hiding this comment.
Seems like there's some repetition, and this query arg is always here, regardless of whether we specify a path or not. I'd deduplicate.
| @@ -16,7 +16,9 @@ export default function useNavigateToEntityRecord() { | |||
|
|
|||
| const onNavigateToEntityRecord = useCallback( | |||
| ( params ) => { | |||
| history.push( { ...params, focusMode: true, canvas: 'edit' } ); | |||
| history.navigate( | |||
| `/${ params.postType }/${ params.id }?canvas=edit&focusMode=true` | |||
There was a problem hiding this comment.
Doing the query building feels a bit tedious, especially if we're handling the encoding ourselves. Should we consider using addQueryArgs() or similar in all these?
There was a problem hiding this comment.
I already use addQueryArgs when the query is dynamic (variables), in this particular case, it feels unnecessary to me.
There was a problem hiding this comment.
While it might feel like overkill, it's just a best practice I recommend. Why? Because it's very easy for someone to pick this up, duplicate it somewhere, and change =edit with a dynamic value, but forget to call addQueryArgs(), and this would result in an inadvertently unencoded value. Just my €0.02 of course.
| }, [ routes, rawQuery, pathArg ] ); | ||
| } | ||
|
|
||
| export function RouterProvider( { |
There was a problem hiding this comment.
What's our current fallback in case someone tries to access a route that doesn't exist?
There was a problem hiding this comment.
It seems to land always in the home page in trunk. Feels a bit weird.
There was a problem hiding this comment.
At least it's not a SEO problem, but still, we might want to implement a 404 page at some point.
| locationMemo.set( location, locationWithQuery ); | ||
| } | ||
| return locationWithQuery; | ||
| } | ||
|
|
||
| export function useLocation() { |
There was a problem hiding this comment.
Should we introduce a check and bail if the context doesn't exist?
There was a problem hiding this comment.
you mean throw an error or what kind of "bail" behavior are you thinking about?
There was a problem hiding this comment.
Could be a warning() or could be an actual error thrown - I lean towards the warning as it's an error targeted towards developers and just throwing might not be the best experience, given how we catch those errors in the editor.
| } | ||
|
|
||
| const RoutesContext = createContext< Match | null >( null ); | ||
| export const ConfigContext = createContext< Config >( { pathArg: 'p' } ); |
There was a problem hiding this comment.
The pathArg field could be generalized into something called pathSource. And support getting path both from query string and the actual path. You have something similar implemented already in the add_rewrite_rule commit. Options:
pathSource={ query( 'p' ) }(orsearchParams( 'p' )) gets the path from query stringpathSource={ pathname( { basePath: ... } ) }gets it fromlocation.pathname, relative to optionalbasePath. IfbasePathis/nested-site/wp-admin/design/styles, then the resultingpathshould be just/styles.
A config like this easily supports all possible scenarios.
There was a problem hiding this comment.
We need to both "get" and "set" the path + query. I agree an abstraction is doable but I think it's a bit early.
There was a problem hiding this comment.
Yes, and in both directions the algorithm would be different. The getter reads from the p query arg that's been already rewritten, but the setter needs to construct a pretty URL.
|
|
||
| export function useLocation() { | ||
| return useContext( RoutesContext ); | ||
| } | ||
|
|
||
| export function useHistory() { | ||
| return useContext( HistoryContext ); | ||
| const { pathArg, beforeNavigate } = useContext( ConfigContext ); |
There was a problem hiding this comment.
This could be a good case for the useEvent helper 🙂 The latest version of the callback is stored to a ref, navigate always calls the latest version, and you get rid of a useMemo dependency.
| document.documentElement.classList.remove( | ||
| options.transition ?? '' | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
Could the transition code be extracted into some form of plugin middleware or a filter like beforeNavigate? It doesn't look very generic, and could help us design a better extension API for the router. The code can be part of the default configuration, but written as a plugin and overridable.
There was a problem hiding this comment.
How is it not generic? For me it's actually very generic. You want to apply a "transition" when navigating, you pass it as an arg to navigate. It can be extracted of course but for me it's something intrinsic to routing.
| matcher.add( [ { path: route.path, handler: route } ], { | ||
| as: route.name, | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
It's not good that we're creating the RouteRecognizer from scratch on each navigation. It could be more constant. Or extracted from the useMatch hook and available imperatively on the history object.
Related #60584
This PR makes our routing framework a bit more solid and getting us closer to opening the registration API publicly.
The main changes in the PR include:
matchfunction in route definition with apathoption. I initially considered using pretty permalinks in the browser url (examplewp-admin/design/stylesto access the styles page in the site editor) but because URL rewrites might not be supported in all WP instances, I left this part out of the PR.pathin routes definitions means we have to change the urls of the site editor a little bit. I'm using a specialpargument for the path in the query string (because I can't use url rewrites). I've included redirects for backwards compatibility of old urls.areasandwidthconfigs of the routes can now be functions that receive the current location (query and params) as arguments. This was done to avoid separating the routes for quick edit, list view... I think this makes our routes way more clear and prevents us from having "convenient" routes, instead we can think about "1 Path" = "1 route"Todo