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.

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

Jump to bottom

Backport global styles custom css changes #3896

Closed
wants to merge 24 commits into from
Closed

Backport global styles custom css changes #3896

wants to merge 24 commits into from

Conversation

glendaviesnz
Copy link

@glendaviesnz glendaviesnz commented Jan 23, 2023 •
edited
Loading

Trac ticket: https://core.trac.wordpress.org/ticket/57536

Combines changes from WordPress/gutenberg#46141 & WordPress/gutenberg#47396

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@glendaviesnz glendaviesnz mentioned this pull request Jan 23, 2023
Merged
@glendaviesnz
Copy link
Author

Still draft as need to look at also bringing across changes from #3896, and also adding some tests

@Mamaduka Mamaduka mentioned this pull request Jan 25, 2023
Closed
85 tasks
oandregal
oandregal reviewed Jan 26, 2023
View reviewed changes
src/wp-includes/global-styles-and-settings.php Outdated
@@ -106,7 +107,7 @@ function wp_get_global_stylesheet( $types = array() ) {
if ( empty( $types ) && ! $supports_theme_json ) {
$types = array( 'variables', 'presets', 'base-layout-styles' );
} elseif ( empty( $types ) ) {
$types = array( 'variables', 'styles', 'presets' );
Copy link
Member

Choose a reason for hiding this comment

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

So. Back at Gutenberg I prepared a PR to update the PHPDocs at WordPress/gutenberg#46817

This is being backported at #3918 though I had to remove the custom-css bit from that PR because it had no landed yet. Perhaps it makes more sense to merged the changes at #3918 in this PR and do it altogether?

Copy link
Member

Choose a reason for hiding this comment

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

Or merging #3918 fast and then rebase this one. I'm fine with whatever works best for people.

Copy link

Choose a reason for hiding this comment

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

I think it will be faster to merge #3918 (@oandregal 's PR) and rebase here, since it's still a draft.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the update - good call to go ahead with your PR - this one is still draft as we are still exploring a change in the custom-css load order which will affect this.

@aristath aristath mentioned this pull request Jan 27, 2023
Closed
@glendaviesnz glendaviesnz force-pushed the backport/global-styles-custom-css branch from 5205998 to d8f547a Compare January 31, 2023 23:36
@glendaviesnz glendaviesnz marked this pull request as ready for review February 1, 2023 00:04
@glendaviesnz
Copy link
Author

@aristath - I have updated this to match the changes merged in with WordPress/gutenberg#47396, which will affect #3925 also

@glendaviesnz
Copy link
Author

@Mamaduka have pulled across WordPress/gutenberg#47062, but some unit tests are failing on multisite which may be related to this and I ran out of time to work out why - will take another look tomorrow if you don't have time to look at it.

@Mamaduka
Copy link
Member

Mamaduka commented Feb 1, 2023 •
edited
Loading

@glendaviesnz, I was able to resolve failing multisite tests by granting capabilities to admin users.

I added the following to WP_REST_Global_Styles_Controller_Test::wpSetupBeforeClass

if ( is_multisite() ) {
	grant_super_admin( self::$admin_id );
}

P.S. I can't push changes to your branch.

@glendaviesnz glendaviesnz force-pushed the backport/global-styles-custom-css branch from 0ca55f9 to 400d437 Compare February 1, 2023 20:39
@Mamaduka
Copy link
Member

Mamaduka commented Feb 1, 2023

@glendaviesnz, we should probably place the grant_super_admin part of the code after $admin_id is assigned.

hellofromtonya
hellofromtonya previously requested changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

To ensure not to forget, I'm blocking the commit of this PR until it is tested in the WP Admin area. Why? The wp_cache_*() functions are loaded into memory or available when load-styles.php runs. If the new wp_get_global_styles_custom_css() runs during this cycle, a fatal error and/or unstyled admin area will happen.

Testing instructions are found here #3712 (comment).

This test is mandatory before committing. Else it could break wp.org.

Also testing in the WP admin area to determine if a fatal error or breaks in CSS happen from not having wp_cache_*() functions available when load-styles.php runs. See the testing instructions here for this necessary test. I'll

glendaviesnz and others added 4 commits February 2, 2023 11:30
@glendaviesnz @hellofromtonya
Remove unnecessary description
d6c6e65 
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
@glendaviesnz @hellofromtonya
But make user definition that was removed by mistake
20442c9 
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
@glendaviesnz
Change ordering of $request[styles] assignment to $config[styles]
d055fa5
@glendaviesnz @hellofromtonya
Change cache key and add doc comments
d37e7f2 
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
felixarntz
felixarntz reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@glendaviesnz Mostly looks solid to me, just a few points of feedback. One thing is potentially more critical and should be considered for a backward compatible solution (from an end user perspective).

src/wp-includes/global-styles-and-settings.php Outdated Show resolved Hide resolved
src/wp-includes/global-styles-and-settings.php Show resolved Hide resolved
src/wp-includes/global-styles-and-settings.php Outdated Show resolved Hide resolved
src/wp-includes/script-loader.php Show resolved Hide resolved
@glendaviesnz @hellofromtonya
match cache key to method name
d71a7fe 
Co-authored-by: Tonya Mork <tonya.mork@automattic.com>
@glendaviesnz
Copy link
Author

Thanks @felixarntz and @hellofromtonya for the detailed feedback!

dream-encode
dream-encode reviewed Feb 1, 2023
View reviewed changes
Copy link
Contributor

@dream-encode dream-encode left a comment

Choose a reason for hiding this comment

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

Sorry, I reviewed this a couple of days ago, but didn't finish.

src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php Outdated Show resolved Hide resolved
felixarntz
felixarntz reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@glendaviesnz Code-wise this looks good to go from my perspective.

The only blocking thing from my perspective is what @hellofromtonya shared in #3896 (review). We need to test that this does not interfere with the load-styles.php request, prior to commit.

Looking at the code paths, I believe it does not, but we should double-check by following the test methodology linked in @hellofromtonya's comment.

@glendaviesnz
Copy link
Author

glendaviesnz commented Feb 2, 2023 •
edited
Loading

@hellofromtonya I have run the suggested tests, re load-styles.php:

Step 1: Added the following constants to wp-config.php:

define( 'CONCATENATE_SCRIPTS', true );
define( 'WP_DEBUG', false );
define( 'SCRIPT_DEBUG', false );

Step 2: Applied this PR
Step 3: Started localhost environment with

npm install
npm run build:dev
npm run env:start
npm run env:install

Step 4: Logged in and opened the WP Admin area.

✅ No fatal error occured
✅ The load-styles.css stylesheet loaded
✅ Navigated through screens and no fatal error occured

load-styles

@hellofromtonya hellofromtonya dismissed their stale review February 2, 2023 14:06

Testing has confirmed no impact to load-styles.php. Dismissing this blocking review as it's no longer necessary.

hellofromtonya
hellofromtonya reviewed Feb 2, 2023
View reviewed changes
src/wp-includes/class-wp-theme-json.php
*
* @since 6.2.0
*
* @return string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return string
* @return string The global styles custom CSS.

Copy link
Member

Choose a reason for hiding this comment

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

@hellofromtonya Addressed this as part of follow up 2cf1b08.

hellofromtonya
hellofromtonya reviewed Feb 2, 2023
View reviewed changes
src/wp-includes/block-editor.php Outdated
@@ -408,6 +408,12 @@ function get_block_editor_settings( array $custom_settings, $block_editor_contex
$block_classes['css'] = $actual_css;
$global_styles[] = $block_classes;
}
// Add the custom CSS as separate style sheet so any invalid CSS entered by users does not break other global styles.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is too long on one line.

Suggested change
// Add the custom CSS as separate style sheet so any invalid CSS entered by users does not break other global styles.
/*
* Add the custom CSS as separate style sheet so any invalid CSS entered
* by users does not break other global styles.
*/

Helps to improve readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. I think it needs an "a" before the word "separate".
  2. Also, isn't "style sheet" one word, e.g. stylesheet?

Copy link
Member

Choose a reason for hiding this comment

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

I can take care of committing this and address this feedback prior to the commit.

@hellofromtonya
Copy link
Contributor

I think (correct me if I'm wrong) this PR is good shape for commit.

  • The concern @felixarntz and I had has been tested to validate it's not a concern ✅
  • All feedback (minus a too long to read inline comment request) has been addressed 💹
  • Open questions have been addressed ✅

Is there anything else pending before commit? @glendaviesnz @dream-encode @felixarntz?
If no, could the committers please approve for confirmation? 🙇

dream-encode
dream-encode approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@dream-encode dream-encode left a comment

Choose a reason for hiding this comment

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

Tiny, semantic suggestions, this this looks ready to me.

src/wp-includes/block-editor.php Outdated
@@ -408,6 +408,12 @@ function get_block_editor_settings( array $custom_settings, $block_editor_contex
$block_classes['css'] = $actual_css;
$global_styles[] = $block_classes;
}
// Add the custom CSS as separate style sheet so any invalid CSS entered by users does not break other global styles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. I think it needs an "a" before the word "separate".
  2. Also, isn't "style sheet" one word, e.g. stylesheet?

hellofromtonya
hellofromtonya approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

Ready to go. Felix will take care of David's review during commit. 👍 from me

@felixarntz
Copy link
Member

Committed in https://core.trac.wordpress.org/changeset/55192

@felixarntz felixarntz closed this Feb 2, 2023
@glendaviesnz
Copy link
Author

Thanks @felixarntz, @hellofromtonya and @dream-encode !

@aristath aristath mentioned this pull request Feb 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oandregal oandregal oandregal left review comments

@felixarntz felixarntz felixarntz approved these changes

@ntsekouras ntsekouras ntsekouras left review comments

@dream-encode dream-encode dream-encode approved these changes

@hellofromtonya hellofromtonya hellofromtonya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@glendaviesnz @Mamaduka @hellofromtonya @felixarntz @oandregal @dream-encode @ntsekouras