-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
core-data: Fix nested property access with undefined name #54790
Conversation
Looking at #48310 I also just noticed https://github.com/WordPress/gutenberg/pull/48310/files#diff-c9806943cd2157856848ecea1d5d7f97f6bd462827ed87b246be86c758f684f8R337 I am unsure what the connection between the JS and TS files is, but this looks very similar: gutenberg/packages/core-data/src/selectors.ts Line 347 in 61736f4
Might this be related and also need a fix? |
@kraftner, both paths have been fixed in this PR. |
Size Change: +1 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Sorry, my brain apparently just misfired overlooking the second change. 🤪 Sorry for the noise. 😬 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected ✅
The regression was introduced in WP 6.3, I'm tentatively marking this for backport in next major release.
Sidenote: It might be a good idea to introduce more high-level tests for core data selectors. There have been previous cases when unit tests were insufficient to catch regressions like this.
@@ -344,7 +344,7 @@ export const getEntityRecord = createSelector( | |||
const field = fields[ f ].split( '.' ); | |||
let value = item; | |||
field.forEach( ( fieldName ) => { | |||
value = value[ fieldName ]; | |||
value = value?.[ fieldName ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be nice to abort early when value
becomes undefined
. Also, this can be a good use of reduce
:
const value = fields.reduce( ( v, f ) => v?.[ f ], item );
But the early abort probably requires a for
loop with break
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but FWIW I opted for brevity here. And I think we no longer need to abort early since we're covering undefined
with the optional chaining.
I agree. 👍 I've added a unit test that covers this particular scenario in de11c87 but it might be a good idea to add some more tests in the future. |
* core-data: Fix nested property access with undefined name * Add a unit test
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 084cfc2 |
* core-data: Fix nested property access with undefined name * Add a unit test
* use the wporg cdn (#54795) * core-data: Fix nested property access with undefined name (#54790) * core-data: Fix nested property access with undefined name * Add a unit test * Social Links: add X (#54092) * Social Links: add X Fixes #53223 * Add Twitter keyword to variation This will allow people to find the new icon when searching for Twitter. See #53223 (comment) * Reorder links alphabetically Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> * No need for a capital letter Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> * Fix svg attributes See #54092 (comment) Co-authored-by: Rich Tabor <hi@richtabor.com> * Remove "icon" Co-authored-by: Nick Diego <nickmdiego@gmail.com> * Update X icon path See #54092 (comment) See #54092 (comment) --------- Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Nick Diego <nickmdiego@gmail.com> * remove font files created by tests after tests run (#54771) * Check that new pattern is synced before replacing existing blocks with a synced pattern (#54804) * Patterns: Improve sentence case consistency of labels and notices (#54807) * Navigation block: fix padding on mobile overlay when global padding is 0 (#53725) * force min value for padding to be 2rem * fallback for when the css variables are not defined * Allow the padding to be smaller than 2rem * Add fix to avoid trigger hover state on links when overlay opens --------- Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> * Always show the total number of patterns even with only one page (#54813) * Always show the total number of patterns even with only one page * Add to explorer too * Hide total number of 0 * Font Library: Avoid rendering Font Library UI outside Gutenberg plugin (#54830) * Remove action to fix tests. (#54806) * Conditionally remove deprecated 'print_emoji_styles' (#54828) * Fix Performance tests * Fix global styles revision --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Nick Diego <nickmdiego@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Maggie <maggie.cabrera@automattic.com> Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
What?
This PR fixes a regression we introduced in #48310 when migrating away from
_.get()
.The issue is thoroughly explained in #54735.
Fixes #54735, cc @kraftner.
Why?
Fixes an issue with how we're handling
getEntityRecord()
calls withundefined
name
s.How?
We're adding optional chaining so entities with
undefined
name won't throw an error.Testing Instructions
wp.data.select( 'core' ).getEntityRecord( 'root', '__unstableBase', undefined, { _fields: [ 'routes./wp/v2/posts' ] } )
in your console.Testing Instructions for Keyboard
None.
Screenshots or screencast
None.