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

Plugin: Correctly classify functionality in the lib folder #39972

Merged
merged 7 commits into from
Apr 4, 2022

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 1, 2022

What?

Part of #39889 that lists the changes to backport for the WordPress 6.0 release.

Related Site Editor core merge commit WordPress/wordpress-develop@f034bc8.
Related Trac ticket for the navigation editor https://core.trac.wordpress.org/ticket/50544.

Why?

We need to correctly classify added code changes so we know exactly whether they are still experimental, they were shipped in WordPress 5.9 or they should be backported to WordPress core as part of 6.0 major release.

How?

I moved files from the lib folder to subfolders after checking whether they are still experimental, were backported to WordPress 5.9 or I believe they should be backported to WordPress 6.0.

Experimental

WordPress 5.9

WordPress 6.0

Testing Instructions

No changes at all. The plugin should work as before.

@gziolo gziolo assigned adamziel and gziolo and unassigned adamziel Apr 1, 2022
@gziolo gziolo added Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Code Quality Issues or PRs that relate to code quality [Status] In Progress Tracking issues with work in progress labels Apr 1, 2022
@gziolo gziolo requested a review from oandregal as a code owner April 1, 2022 09:49
* @param WP_REST_Response $response Response data served by the WordPress REST index endpoint.
* @return WP_REST_Response
*/
function gutenberg_register_site_logo_to_rest_index( $response ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

good call adding the gutenberg_prefix

Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

LGTM, I restarted the failing E2E test that, again, seems to be just flaky.

.github/CODEOWNERS Outdated Show resolved Hide resolved
@gziolo gziolo force-pushed the update/lib-triage branch 3 times, most recently from 268dccd to 563e51a Compare April 1, 2022 11:46
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Apr 1, 2022
@gziolo
Copy link
Member Author

gziolo commented Apr 1, 2022

@noisysocks, @talldan, and @anton-vlasenko, can you confirm whether all changes applied to navigation.php were fully backported for WordPress 5.9 release? I definitely can find some changes in this commit WordPress/wordpress-develop@f034bc8. The question is whether we still need to add some changes as part of WordPress 6.0.

I'd like also to confirm that all code in navigation-theme-opt-in.php is still experimental. Functions reference still open Trac ticker: https://core.trac.wordpress.org/ticket/50544, so I assumed it's still not part of WordPress core. The same question applies to navigation-page.php.

@gziolo
Copy link
Member Author

gziolo commented Apr 4, 2022

I updated more code in 9bc946f:

@gziolo
Copy link
Member Author

gziolo commented Apr 4, 2022

I'm pretty happy about the current folder structure:

Screenshot 2022-04-04 at 10 18 43

I tried removing pwa.php and service-worker.js in #39930 but there were some surprising e2e test errors so I reverted it. I will try again next week.

With bfa2120 I moved lib/style-engine to lib/experimental/style-engine.

With 7d631be I moved lib/rest-api.php to lib/experimental/rest-api.php and extracted and moved some code to lib/wordpress-5.9/rest-api.php.

I also renamed lib/wordpress-6.0/utils.php to lib/wordpress-6.0/functions.php to align with the final location in WordPress core.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Really cool PR :) Thanks for doing that.

For the edit-site-page.php I'm not sure whether we can just move it as is to 5.9 lib. It seems there's some changes in the "preloading" in 6.0. So I guess either we move the file to 6.0 or find a way to override the preloading stuff in 6.0 folder.

*
* @param WP_Scripts $scripts WP_Scripts instance.
*/
function gutenberg_register_vendor_scripts( $scripts ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With React 18, we might need to move that to another folder, but it's a problem for later :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I took that into account. It's probably fine to move it between WP versions. When SCRIPT_DEBUG is enabled I inject the dev scripts for React Fast Refresh so I will think how to add it to WP core so we don't have to include that every time we update the React version.

@gziolo
Copy link
Member Author

gziolo commented Apr 4, 2022

For the edit-site-page.php I'm not sure whether we can just move it as is to 5.9 lib. It seems there's some changes in the "preloading" in 6.0. So I guess either we move the file to 6.0 or find a way to override the preloading stuff in 6.0 folder.

There is a hook that targets only the preloading functionality. I will land this PR to help coordinate backports to WordPress core and see how we can attack the preloading in a more granular way 💯

@gziolo gziolo merged commit e102e3f into trunk Apr 4, 2022
@gziolo gziolo deleted the update/lib-triage branch April 4, 2022 10:12
@github-actions github-actions bot added this to the Gutenberg 13.0 milestone Apr 4, 2022
@gziolo
Copy link
Member Author

gziolo commented Apr 4, 2022

For the edit-site-page.php I'm not sure whether we can just move it as is to 5.9 lib. It seems there's some changes in the "preloading" in 6.0. So I guess either we move the file to 6.0 or find a way to override the preloading stuff in 6.0 folder.

There is a hook that targets only the preloading functionality. I will land this PR to help coordinate backports to WordPress core and see how we can attack the preloading in a more granular way 💯

It looks like @jsnajdr has covered everything for WordPress 5.9 with this hook:

function optimize_preload_paths( $preload_paths ) {
// remove preload of the `/` route.
$root_index = array_search( '/', $preload_paths, true );
if ( false !== $root_index ) {
array_splice( $preload_paths, $root_index, 1 );
}
// change `/types` context from `edit` to `view` (requested in `loadPostTypeEntities`).
$types_index = array_search( '/wp/v2/types?context=edit', $preload_paths, true );
if ( false !== $types_index ) {
$preload_paths[ $types_index ] = '/wp/v2/types?context=view';
}
// start preloading `/taxonomies` in `view` context (requested in `loadTaxonomyEntities`).
$tax_index = array_search( '/wp/v2/taxonomies?per_page=-1&context=edit', $preload_paths, true );
if ( false === $tax_index ) {
array_push( $preload_paths, '/wp/v2/taxonomies?context=view' );
} else {
$preload_paths[ $tax_index ] = '/wp/v2/taxonomies?context=view';
}
// start preloading `/settings`.
$settings_index = array_search( '/wp/v2/settings', $preload_paths, true );
if ( false === $settings_index ) {
array_push( $preload_paths, '/wp/v2/settings' );
}
// modify the preload of `/users/me` to match the real request.
foreach ( $preload_paths as $user_index => $user_path ) {
if ( is_string( $user_path ) && 0 === strpos( $user_path, '/wp/v2/users/me' ) ) {
$preload_paths[ $user_index ] = '/wp/v2/users/me';
break;
}
}
return $preload_paths;
}
add_filter( 'block_editor_rest_api_preload_paths', 'optimize_preload_paths' );

@jasmussen
Copy link
Contributor

I think this PR may have accidentally broken some of the Site Logo functionality. Before this PR, the W would be replaced by any site logo set, like so:

Screenshot 2022-04-04 at 17 29 06

After this PR, that's broken:

Screenshot 2022-04-04 at 17 29 47

Steps to reproduce:

  • Insert the site logo block
  • Add a site logo
  • Save and publish
  • Reload the editor

The W should be replaced by a round-rectangle site logo instead.

Comment on lines -116 to -123
function register_site_icon_url( $response ) {
$data = $response->data;
$data['site_icon_url'] = get_site_icon_url();
$response->set_data( $data );
return $response;
}

add_filter( 'rest_index', 'register_site_icon_url' );
Copy link
Member Author

@gziolo gziolo Apr 4, 2022

Choose a reason for hiding this comment

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

@jasmussen, great catch. I accidentally removed this filter related to the Site Logo block 😱

I will open PR as the first thing in the morning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ready in #40041.

gziolo added a commit that referenced this pull request Apr 5, 2022
Regression introduced in the `lib` folder refactoring #39972.
gziolo added a commit that referenced this pull request Apr 5, 2022
* Site Logo: Fix adding the site icon
Regression introduced in the `lib` folder refactoring #39972.

* Fix formatting issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts [Type] Code Quality Issues or PRs that relate to code quality
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants