| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Template activation: redirect theme templates urls to wp_registered_template #72003
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. |
|
Flaky tests detected in ada2bb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18289314295
|
| @@ -0,0 +1,38 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took this pattern from
| function gutenberg_get_site_editor_redirection() { |
ellatrix
force-pushed
the
fix/redirect-theme-template-urls
branch
from
0e17dd5 to
ada2bb0
Compare
ellatrix
added
the
Backport to Gutenberg RC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the redir, but we should update the links throughout the admin soon, right?
| } | ||
|
|
||
| // The following redirects are for the new permalinks in the site editor. | ||
| if ( isset( $_GET['p'] ) && preg_match( '/^\/wp_template\/([a-zA-Z0-9-]+\/\/[a-zA-Z0-9-]+)$/', $_GET['p'], $matches ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any point in using $_REQUEST instead? Also, probably a good place to use a different delimiter for the PCRE pattern:
| if ( isset( $_GET['p'] ) && preg_match( '/^\/wp_template\/([a-zA-Z0-9-]+\/\/[a-zA-Z0-9-]+)$/', $_GET['p'], $matches ) ) { | |
| if ( isset( $_GET['p'] ) && preg_match( '_^/wp_template/([a-zA-Z0-9-]+//[a-zA-Z0-9-]+)$_', $_GET['p'], $matches ) ) { | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a friendly bonus, I also suggest adding a comment above with the overall shape of the redirection, to make scanning the code easier. Something like:
// /wp_template/tt5//home -> /wp_registered_template/tt5//homeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we use $_REQUEST? Sure, we can use a different pattern and add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, the suggestion introduced an extra }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry. :)
Why should we use
$_REQUEST?
No good reason comes to mind, but the fact that gutenberg_get_site_editor_redirection did made me wonder. Is there any scenario where we could expect a POST request to need redirection? Put differently: could it hurt to use $_REQUEST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the suggested pattern doesn't work 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched it to use #
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, I chose poorly: there are obviously underscores in the pattern. But my point still stands:
if ( preg_match( '#^/wp_template/([a-zA-Z0-9-]+//[a-zA-Z0-9-]+)$#', $input, $matches ) ) {There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched it to use #
Jinx :)
(Also, when will GitHub finally properly update comment threads in real time...?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put differently: could it hurt to use $_REQUEST?
Done, I guess it doesn't
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
…emplate (#72003) Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org> Co-authored-by: westonruter <westonruter@git.wordpress.org>
cbravobernal
removed
the
Backport to Gutenberg RC
|
Backported for Gutenberg 21.8 in 5fcf384 |
…emplate (WordPress#72003) Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org> Co-authored-by: westonruter <westonruter@git.wordpress.org>
What?
Closes #72151
The urls for theme templates have changed from
/wp_template/twentytwentyfive//hometo/wp_registered_template/twentytwentyfive//home. Let's add redirects.Why?
How?
Adds redirects server side.
Testing Instructions
Visit URLs like
https://localhost:8888/wp-admin/site-editor.php?canvas=edit&p=%2Fwp_template%2Ftwentytwentyfive%2F%2FhomeAlternatively, click the Edit Site admin bar button on the front-end, which still uses the old URL.
Testing Instructions for Keyboard
Screenshots or screencast