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

Global Styles: Fix block spacing values being stripped for users without unfiltered_html capability #3742

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Dec 13, 2022

This PR backports WordPress/gutenberg#46388 and WordPress/gutenberg#46712 which resolved WordPress/gutenberg#45520 in Gutenberg.

To recap: for users without the unfiltered_html capability (e.g. an Admin user within a multi site WordPress instance), when saving custom block spacing values or content/wide layout sizes within global styles, the value is stripped on save. The fix is to ensure that indirect properties (that is, Theme JSON properties that are not directly output via compute_style_properties within remove_insecure_styles and remove_insecure_settings) are also allowed.

For more context, please see: WordPress/gutenberg#46388

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

Testing instructions

  1. To quickly test, add the following somewhere (e.g. at the bottom of src/wp-includes/default-filters.php): add_action( 'init', 'kses_init_filters' ); — this switches on the kses filters used when a user does not have the unfiltered_html capability.
  2. With a blocks theme active, go to global styles within the site editor, and adjust block spacing within layout. When you go to save a value, without this PR, the value will revert to what it was when you originally opened the site editor.
  3. With this PR applied, the value should save correctly.
  4. You can repeat this process at the block level in global styles, with a block that supports block spacing/ block gap, e.g. Social Icons/Links.
  5. Also, you can repeat this process with the content and wide layout sizes within global styles. With this PR applied, updates to content / wide layout sizes should also now be respected when saved.

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.

Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

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

Thanks for handling this @andrewserong! I've left a few thoughts below.

tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJson.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json.php Outdated Show resolved Hide resolved
@andrewserong
Copy link
Contributor Author

Thanks for the feedback @costdev, all good notes! I've implemented those changes. I'm wrapping up for the day, but happy to do any other follow-ups tomorrow (and of course, anyone with access, feel free to jump in and make any changes you might need 😀)

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

These changes make sense to me, I've asked colin to clarify something inline with the covers annotation.

peterwilsoncc
peterwilsoncc previously approved these changes Dec 14, 2022
@andrewserong andrewserong changed the title Global Styles: Fix block spacing values being stripped for uses without unfiltered_html capability Global Styles: Fix block spacing values being stripped for users without unfiltered_html capability Dec 15, 2022
@andrewserong
Copy link
Contributor Author

Just an update that I'm wrapping up for the year now, so likely won't catch continued discussion here. If this change looks suitable, though, please feel free to go ahead and commit it if anyone would like to. I think ideally this would be a good one to make it into 6.1.2 if it can, rather than waiting until 6.2. Thanks again for the reviews!

@mmtr
Copy link

mmtr commented Dec 23, 2022

Just a heads up that I just merged WordPress/gutenberg#46712 in Gutenberg which is a follow-up of the changes backported here, so I think we could use this PR to include the follow-up as well.

@peterwilsoncc
Copy link
Contributor

That makes sense @mmtr

I've dismissed my review pending any follow up. I'm not sure when Andrew returns from EOY holidays so someone else may need to create a follow up PR with both issues.

@andrewserong
Copy link
Contributor Author

Thanks for the heads-up @mmtr and Peter! I'm back today, so I'll roll those changes into this PR — if I don't get to it today, I'll fix it up tomorrow.

@andrewserong andrewserong force-pushed the fix/global-spacing-stripped-block-spacing-values branch from 64c4621 to b7f4a5d Compare January 3, 2023 05:05
@andrewserong
Copy link
Contributor Author

Alrighty, I've merged in the changes from WordPress/gutenberg#46712 and re-tested locally, and it appears to all be working well for me, so this should be ready for another look now.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

Thanks Andrew, still looks good.

@mmtr
Copy link

mmtr commented Jan 9, 2023

Thanks @andrewserong for backporting WordPress/gutenberg#46712 as well!

Co-authored-by: Miguel Torres <miguelmariatorresrojas@gmail.com>
* but are used elsewhere in the processing of global styles. The indirect
* property is used to validate whether or not a style value is allowed.
*
* @since 6.1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

@peterwilsoncc is this planned for 6.1.2? Or is this for 6.2?

@andrewserong
Copy link
Contributor Author

Thanks for tidying this one up @hellofromtonya! Because this resolves a bug in 6.1, the hope was that this could make it into 6.1.2 (and therefore also 6.2).

@peterwilsoncc
Copy link
Contributor

@andrewserong @hellofromtonya If the bug, or relevant block, was introduced in 6.1 then I'm happy for this to go in the next minor release too.

@andrewserong
Copy link
Contributor Author

@Mamaduka @ntsekouras just double-checking the status of this one being included in 6.2? Ideally the fix should land for both 6.1.2 and 6.2.

@Mamaduka
Copy link
Member

Mamaduka commented Feb 8, 2023

@andrewserong, I believe the fix is merged into the trunk first and then backported into the minor branches.

@hellofromtonya
Copy link
Contributor

@peterwilsoncc did you want to commit this for 6.2 and then backport to 6.1 branch?

@peterwilsoncc
Copy link
Contributor

Committed in r55345 / 4d772a5

@andrewserong
Copy link
Contributor Author

Wonderful, thanks for committing, Peter! 🙇

@oandregal
Copy link
Member

👋 There are some changes here that are not part of the original gutenberg PR. I've backported them to gutenberg at WordPress/gutenberg#48646

@andrewserong
Copy link
Contributor Author

Thanks so much @oandregal! I'll give that PR a review today.

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.

Unable to change the block spacing within Global Styles when KSES is active
7 participants