Skip to content
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

Redirect old theme_page Site Editor routes to the new site-editor.php page #42643

Merged
merged 7 commits into from
Jul 26, 2022

Conversation

fullofcaffeine
Copy link
Member

@fullofcaffeine fullofcaffeine commented Jul 22, 2022

What?

Redirect the following routes:

  • admin.php?page=gutenberg-edit-site
  • themes.php?page=gutenberg-edit-site

To: site-editor.php

Why?

Gutenberg 13.7 deprecated the theme_page-based routes (see above) for the Site Editor in this PR: #41306. However, third-party code (i.e from plugins -- ex: Jetpack) might still reference the old routes. We don't want their flows to break with the 13.7 upgrade, and a good transient solution is to keep the old routes around while making them redirect to the new route.

What about the definitive solution?

I don't know what is the current backwards-compatibility policy for WordPress/Gutenberg, but it appears that we should be more careful about deprecating routes/paths. Ideally, routes are abstracted in helper methods and not hardcoded throughout the codebase. For example, let's say that the Site Editor route was wrapped in a site_editor_url helper method from the beginning. The helper would be defined in GB or WP version (if backported). Third-party consumers would (hopefully) be using that, and any underlying changes would be picked up automatically. This is more of food for thought for future route additions. Unfortunately, for the Site Editor, adding the helper now won't solve the immediate problem.

To entirely deprecate the old paths and get rid of the redirect, third-party consumers would need to add a conditional to check for the GB version and potentially the WP version, too (since the code might be backported to WP in the future, AFAIK). This is a possibility, and the effort would depend on how many third-party consumers are already referencing the old paths.

Or we could add this redirect and then add a route abstraction as suggested above so that any future changes won't need any conditionals or redirects anymore.

What are your thoughts?

How?

Adds a new "theme_page" if the theme is block_theme. This seems to whitelist the route and allows it to be accessible. Without it, we get a permission error from WordPress. It's added as a callback to the admin_menu action.

Then, we add a callback to the load-appearance_page_gutenberg-edit-site action that finally redirects. From what I understand, this action is triggered when the aforementioned "theme_page" is loaded, and then the redirect takes place.

There's a problem for now, though, which I'm still trying to solve: the add_theme_page adds a new submenu for the Editor. We end up with two items under appearance :( ... is there a way to whitelist the routes without using add_theme_page?

I mainly found the code that worked by trial and error, so I would appreciate thoughts about whether there are any other better ways to accomplish it.

Testing Instructions

  • Build GB from this branch, fire up wp-env, login to the WP instance;
  • localhost:8889/wp-admin/admin.php?page=gutenberg-edit-site should redirect to localhost:8889/wp-admin/site-editor.php
  • localhost:8889/wp-admin/themes.php?page=gutenberg-edit-site should redirect to localhost:8889/wp-admin/admin/site-editor.php

Screenshots or screencast

@fullofcaffeine fullofcaffeine marked this pull request as ready for review July 22, 2022 18:29
@fullofcaffeine
Copy link
Member Author

I'll amend and rename the WIP commit later with a proper description :)

@fullofcaffeine fullofcaffeine force-pushed the add/backwards-compatibility-to-site-editor-routes branch from 341550f to 6b1207b Compare July 22, 2022 18:46
@fullofcaffeine fullofcaffeine marked this pull request as draft July 22, 2022 18:47
@gziolo gziolo requested review from a team and nerrad and removed request for a team July 22, 2022 20:19
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how third party plugins handle their dependencies and what was the goal by using the GB url. Was there the need for adding some links to site editor? Did they made sure that GB was installed and active? Are there checks about the min version of WP or GB supported?

I'm not sure if this is something that should be handled in GB or the plugins that used that, but if we end up needing to keep the redirects, I think it should be commented to be for a limited time to give time to plugins to update their code..

@fullofcaffeine
Copy link
Member Author

fullofcaffeine commented Jul 25, 2022

I'm wondering how third party plugins handle their dependencies and what was the goal by using the GB url. Was there the need for adding some links to site editor? Did they made sure that GB was installed and active? Are there checks about the min version of WP or GB supported?

Unfortunately, I don't know, and I think that given the open nature of the WP ecosystem, it'll be hard to know for sure. I still think that a route change should be done with backward compatibility in mind, similar to the standard way of deprecating API routes -- i.e keep the old route so as not to break existing consumers.

In this case, we possibly wouldn't need to keep the redirect forever, but only as a transient solution while consumers update their code to handle the route changing. The definitive solution could be to update the code for consumers to handle the new route. That will depend on changes in upstream code for any third-party plugins that still use the old routes. This can't be a solution now as we don't have a full list of all the plugins that are affected, so this change will act as a safety-net while upstream devs work on the fix (hopefully), and this will take time (to be honest, though, I still think we should somehow abstract routes to prevent this from happening in the future, see the PR description).

One thing is for sure: we will need to support GB < 13.7 so we need to keep supporting the old routes and now with 13.7, the new route.

@fullofcaffeine fullofcaffeine force-pushed the add/backwards-compatibility-to-site-editor-routes branch from 6b1207b to 551d3a1 Compare July 25, 2022 18:31
@fullofcaffeine fullofcaffeine marked this pull request as ready for review July 25, 2022 18:31
@fullofcaffeine fullofcaffeine force-pushed the add/backwards-compatibility-to-site-editor-routes branch from 551d3a1 to fb817a7 Compare July 25, 2022 18:36
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some wording nitpicks 🙇🏻

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me 🚢

@ntsekouras ntsekouras merged commit 9ac314e into trunk Jul 26, 2022
@ntsekouras ntsekouras deleted the add/backwards-compatibility-to-site-editor-routes branch July 26, 2022 09:10
@github-actions github-actions bot added this to the Gutenberg 13.8 milestone Jul 26, 2022
@fullofcaffeine
Copy link
Member Author

Thanks, folks!

chad1008 pushed a commit that referenced this pull request Jul 26, 2022
…php` page (#42643)

* Redirect pre-13.7 Site Editor routes to `site.editor.php`

* move file to different compat folder

* update file structure docs

* Update lib/compat/plugin/edit-site-routes-backwards-compat.php

Co-authored-by: George Mamadashvili <[email protected]>

* fix empty rendered menu item

* Apply suggestions from code review

Co-authored-by: Andrei Draganescu <[email protected]>

* Update lib/compat/plugin/edit-site-routes-backwards-compat.php

Co-authored-by: George Mamadashvili <[email protected]>

Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: George Mamadashvili <[email protected]>
Co-authored-by: Andrei Draganescu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants