Skip to content

[GB 13.7] Gutenberg 13.7 Site Editor route fixes #25281

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.

Sign up for GitHub

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

Merged
kraftbj merged 11 commits into from
Aug 1, 2022
Merged

[GB 13.7] Gutenberg 13.7 Site Editor route fixes #25281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
904c9b1
Calipsoify the site-editor.php route, too
fullofcaffeine Jul 26, 2022
a27fc2a
changelog
fullofcaffeine Jul 26, 2022
455a398
Replace other references to the old site editor routes and refactor t…
fullofcaffeine Jul 26, 2022
9692cb7
changelog
fullofcaffeine Jul 26, 2022
b5e366b
Improve changelog filename
fullofcaffeine Jul 27, 2022
afd2be2
Fix linting errors
fullofcaffeine Jul 28, 2022
3e61439
Run tools/fixup-project-versions.sh
fullofcaffeine Jul 28, 2022
69ae1c0
Changelog
fullofcaffeine Jul 28, 2022
59a22ef
Update packages after running tools/fixup-project-versions.sh
fullofcaffeine Jul 28, 2022
f030bb3
Make route logic backward-compatible
fullofcaffeine Jul 29, 2022
2d03003
Make condition more readable
fullofcaffeine Jul 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions projects/js-packages/publicize-components/changelog/gutenberg-13.7-route-fixes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Updated package dependencies.
4 changes: 2 additions & 2 deletions projects/js-packages/publicize-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"private": true,
"name": "@automattic/jetpack-publicize-components",
"version": "0.3.4",
"version": "0.3.5-alpha",
"description": "A library of JS components required by the Publicize editor plugin",
"homepage": "https://github.com/Automattic/jetpack/tree/HEAD/projects/js-packages/publicize-components/#readme",
"bugs": {
Expand All @@ -20,7 +20,7 @@
},
"dependencies": {
"@automattic/jetpack-components": "workspace:* || ^0.17",
"@automattic/jetpack-shared-extension-utils": "workspace:* || ^0.5",
"@automattic/jetpack-shared-extension-utils": "workspace:* || ^0.6",
"@wordpress/annotations": "2.12.0",
"@wordpress/components": "19.14.0",
"@wordpress/compose": "5.10.0",
Expand Down
4 changes: 4 additions & 0 deletions projects/js-packages/shared-extension-utils/changelog/gutenberg-13.7-route-fixes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fixed

Change Site Editor route to `site-editor.php`
2 changes: 1 addition & 1 deletion projects/js-packages/shared-extension-utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@automattic/jetpack-shared-extension-utils",
"version": "0.5.0",
"version": "0.6.0-alpha",
"description": "Utility functions used by the block editor extensions",
"homepage": "https://github.com/Automattic/jetpack/tree/HEAD/projects/js-packages/shared-extension-utils/#readme",
"bugs": {
Expand Down
3 changes: 1 addition & 2 deletions projects/js-packages/shared-extension-utils/src/plan-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,8 @@ export function getUpgradeUrl( { planSlug, plan, postId, postType } ) {

return addQueryArgs(
window.location.protocol +
`//${ getSiteFragment().replace( '::', '/' ) }/wp-admin/admin.php`,
`//${ getSiteFragment().replace( '::', '/' ) }/wp-admin/site-editor.php`,
Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

This one will work fine with 13.6 and 13.7. In envs with 13.6, it will redirect to site-editor.php, which will in turn redirect to themes.php .... If 13.7+ is active instead, it will work as is, as 13.7 uses this route for the Site Editor.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how to test this change from an integration standpoint, I asked the Jetpack folks here: p1659139972849529-slack-CDLH4C1UZ. However, the change is simple enough that it's feasible to accept it as is.

{
page: 'gutenberg-edit-site',
postId: queryParams.get( 'postId' ),
postType: queryParams.get( 'postType' ),
plan_upgraded: 1,
Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/gutenberg-13.7-route-fixes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: bugfix

Calipsoify the `site-editor.php` route so that it opens the Site Editor from the Gutenframe. Gutenberg 13.7 deprecated the old routes and uses core's `site-editor.php`.
15 changes: 9 additions & 6 deletions projects/plugins/jetpack/class.jetpack-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,12 +733,15 @@ public static function add_iframed_editor_style() {
return;
}

$allowed_pages = array( 'admin.php', 'themes.php' );
$is_site_editor_page = in_array( $pagenow, $allowed_pages, true ) &&
isset( $_GET['page'] ) && 'gutenberg-edit-site' === $_GET['page']; // phpcs:ignore WordPress.Security.NonceVerification.Recommended

// WP 5.9 puts the site editor in `site-editor.php` when Gutenberg is not active.
if ( 'site-editor.php' !== $pagenow && ! $is_site_editor_page ) {
// Pre 13.7 pages that still need to be supported if < 13.7 is
// still installed.
$allowed_old_pages = array( 'admin.php', 'themes.php' );
$is_old_site_editor_page = in_array( $pagenow, $allowed_old_pages, true ) && isset( $_GET['page'] ) && 'gutenberg-edit-site' === $_GET['page']; // phpcs:ignore WordPress.Security.NonceVerification.Recommended
// For Gutenberg > 13.7, the core `site-editor.php` route is used instead
$is_site_editor_page = 'site-editor.php' === $pagenow;
Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

I think we'll need to keep checking for the old page here if we want this condition to keep working with Gutenberg 13.6 🤔 the problem is that Gutenberg 13.6 will redirect site-editor.php to the old themes.php?page=gutenberg-edit-site.

Copy link
Contributor

@creativecoder creativecoder Jul 29, 2022

Choose a reason for hiding this comment

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

It looks like the existing code handles both Site editor routes, so you can this leave as is?

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@creativecoder Are you sure? Unless I'm missing something, if 13.6 is active, accessing site-editor.php will redirect to themes.php?page=gutenberg-edit-site, so this logic will not apply.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I see what you mean, sorry for the misunderstanding! Yeah, the existing logic does handle both, you're right. However, I think the rewritten one is a bit easier to read and understand.


$should_skip_adding_styles = ! $is_site_editor_page && ! $is_old_site_editor_page;
if ( $should_skip_adding_styles ) {
return;
}

Expand Down
5 changes: 4 additions & 1 deletion projects/plugins/jetpack/modules/masterbar/admin-menu/class-admin-menu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,15 +402,18 @@ public function add_jetpack_menu() {
* Update Site Editor menu item's link and position.
*/
public function add_gutenberg_menus() {
if ( self::CLASSIC_VIEW === $this->get_preferred_view( 'admin.php?page=gutenberg-edit-site' ) ) {
if ( self::CLASSIC_VIEW === $this->get_preferred_view( 'site-editor.php' ) ) {
Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

Do you think we should try to get the preference from the old route if the one for site-editor-php is null (I don't know what is the actual return value returned when it can't find a persisted preferred view, though, would have to look)?

I think that if we don't, users will lose their preference here, if it was set 🤔

Copy link
Contributor

@noahtallen noahtallen Jul 29, 2022

Choose a reason for hiding this comment

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

That makes sense to me 👍

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

Do you know how to set this preference for the site editor? 🤔 cc @Addison-Stavlo

Copy link
Contributor

@creativecoder creativecoder Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I did a little searching trying to understand this. The "preferred view" comes from the preferred-view query parameter in the url. The only place I can see that we set that in the UI is in the View panel at the top right of some Calypso/wp-admin screens.

image

jetpack/projects/plugins/jetpack/modules/masterbar/admin-menu/class-base-admin-menu.php

Line 553 in 6e9b58d

public function add_dashboard_switcher() {

I'm not seeing a way the preferred view would get set specifically for the Site editor, other than manually appending the query parameter to the wp-admin url. If that's correct, I don't think we need to worry about persisting the preference.

Copy link
Contributor

@creativecoder creativecoder Jul 29, 2022

Choose a reason for hiding this comment

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

I miss-typed that last comment! Edited, it now reads: "I don't think we need to worry about persisting the preference."

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

Hey @creativecoder, thanks for diving into this! Yeah, I was having a chat with @jeyip about the very same thing and we reached the same conclusion. It seems that we can assume the Site Editor has always been set to load inside the Gutenframe and that users couldn't change that.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That being said, I'll leave an additional note that - as reported by you here - the Gutenframe isn't able to load the Site Editor in AT sites at the moment in any case, though.

return;
}

$this->update_menu( 'gutenberg-edit-site', 'https://wordpress.com/site-editor/' . $this->domain, null, null, null, 59 );

// Gutenberg 11.9 moves the Site Editor to an Appearance submenu as Editor.
$submenus_to_update = array(
// Keep the old rule in order to Calypsoify the route for GB < 13.7.
'gutenberg-edit-site' => 'https://wordpress.com/site-editor/' . $this->domain,
// New route: Gutenberg 13.7 changes the site editor menu item slug and url.
'site-editor.php' => 'https://wordpress.com/site-editor/' . $this->domain,
);
$this->update_submenus( 'themes.php', $submenus_to_update );
// Gutenberg 11.9 adds an redundant site editor entry point that requires some calypso work
Expand Down
11 changes: 9 additions & 2 deletions projects/plugins/jetpack/modules/notes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,15 @@ public function action_init() {

// Do not show notifications in the Site Editor, which is always in fullscreen mode.
global $pagenow;
// phpcs:ignore WordPress.Security.NonceVerification
if ( 'admin.php' === $pagenow && isset( $_GET['page'] ) && 'gutenberg-edit-site' === $_GET['page'] ) {

// Pre 13.7 pages that still need to be supported if < 13.7 is
// still installed.
$allowed_old_pages = array( 'admin.php', 'themes.php' );
$is_old_site_editor_page = in_array( $pagenow, $allowed_old_pages, true ) && isset( $_GET['page'] ) && 'gutenberg-edit-site' === $_GET['page']; // phpcs:ignore WordPress.Security.NonceVerification.Recommended
// For Gutenberg > 13.7, the core `site-editor.php` route is used instead
$is_site_editor_page = 'site-editor.php' === $pagenow;

if ( $is_site_editor_page || $is_old_site_editor_page ) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion projects/plugins/jetpack/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
"@automattic/jetpack-licensing": "workspace:* || ^0.5",
"@automattic/jetpack-partner-coupon": "workspace:* || ^0.2",
"@automattic/jetpack-publicize-components": "workspace:* || ^0.3",
"@automattic/jetpack-shared-extension-utils": "workspace:* || ^0.5",
"@automattic/jetpack-shared-extension-utils": "workspace:* || ^0.6",
"@automattic/popup-monitor": "1.0.1",
"@automattic/request-external-access": "1.0.0",
"@automattic/social-previews": "1.1.4",
Expand Down