Global styles: Fix error when updating padding input in global styles #49465
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
|
Size Change: +6 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
tellthemachines
approved these changes
Mar 30, 2023
There was a problem hiding this comment.
Thanks, this fixes the issue!
I think the problem is that we we're not checking whether value exists before calling cloneObject with it on line 35. I guess we could have checked value instead, but it seems more foolproof to check if the object exists before recursing on it.
talldan
reviewed
Mar 30, 2023
| @@ -27,7 +27,7 @@ function normalizePath( path ) { | |||
| * @return {*} Cloned object, or original literal non-object value. | |||
| */ | |||
| function cloneObject( object ) { | |||
| if ( typeof object === 'object' ) { | |||
| if ( object && typeof object === 'object' ) { | |||
There was a problem hiding this comment.
Is there a test case that can be added to the tests for immutableSet?
BTW, cloneObject wasn't introduced in #49365, it was already in the codebase. I think it might be from the refactor away from lodash, so it might be good to check what lodash does in this situation and mirror that.
There was a problem hiding this comment.
It looks like this is what happens:
_.set( null, [ 'path' ], 'value' ); // returns `null`
_.set( undefined, [ 'path' ], 'value' ); // returns `undefined`So I think the fix is good in that it still returns the original value.
Ignore this, the bug is different, and immutableSet does work differently to lodash set.
I've added some tests that catch the issue, which was specifically happening when using immutableSet on objects that have a nested null property. (that was mentioned in the PR description, but I read it too quickly 😄 )
|
Flaky tests detected in 1a253a2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4561612597
|
|
Adding @tyxla here as I believe he originally wrote this utility. He might have better insights. |
|
Merging for now so that the bug is solved. The specific implementation can always be revisited. |
tyxla
reviewed
Mar 30, 2023
|
Thanks for merging and the discussion, and for adding in the tests @talldan! 🙇 |
|
Folks, we the Theme Designers from Dotorg are still getting this error for a couple of days now. I was about to create a PR when @mtias pointed me to this one. Are you still on it?
|
|
@henriqueiamarino, which version of Gutenberg are you running? I've just retested adjusting padding in global styles in If you're still able to replicate the issue on trunk, do you mind sharing the error message? (Either copy/paste from the console, or via the Copy Error button). |
andrewserong
added
the
Backport to Gutenberg Minor Release
andrewserong
added a commit
that referenced
this pull request
Apr 11, 2023
…#49465) * Fix padding input in global styles * Add tests --------- Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
andrewserong
removed
the
Backport to Gutenberg Minor Release
|
Is it possible this PR introduced a bug where all global settings are saved in global styles when updating the site layout? Please see here: #53868 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fix an error being thrown when going to update padding values at the root level in global styles.
Why?
A
cloneObjectutility function was added in #49365. It looks like it gets called when updating the padding values in global styles, with anullvalue. Anullvalue in this function results in thetypeofcheck being truthy becausenullis an object. This then throws an error.In this case it looks like
cloneObjectgets called recursively, where it's possible for it be called with anullvalue, so we need an additional truthy check, as the one inimmutableSetisn't quite enough. Thanks @tellthemachines for spotting that!How?
Add a truthy check to the
cloneObjectfunction so thatnulldoesn't cause the code to be executed.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
This is the error state that occurred: