Block bindings: allow only string field types in bindings. #66174
+132
−5
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
SantosGuillamot
added
[Type] Bug
|
Size Change: +7 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
SantosGuillamot
changed the title
cbravobernal
changed the title
| // Don't include private fields. | ||
| key.charAt( 0 ) !== '_' && | ||
| // Only support string types. | ||
| typeof entityMetaValues?.[ key ] === 'string' |
There was a problem hiding this comment.
We could add the filter here:
const metaProperties =
options?.schema?.properties?.meta?.properties;
const stringMetaProperties = Object.fromEntries(
Object.entries( metaProperties ).filter(
( [ , value ] ) => value.type === 'string'
)
);
dispatch.receiveRegisteredPostMeta(
postType,
stringMetaProperties
);
There was a problem hiding this comment.
Hmm, I think it makes more sense to have the check here in packages/editor/src/bindings/post-meta.js. That makes the check more specific to bindings and keeps the related exceptions together, whereas getRegisteredPostMeta seems better to me conceptually as a more agnostic function.
There was a problem hiding this comment.
I believe the conditional should look like this, adding some handling for default values, otherwise valid fields will get skipped over in templates where entityMetaFields is undefined due to postId being undefined. Consequently, this also fixes some broken tests.
It might cause other things to break, though — I haven't been able to confirm that yet:
if (
// Don't include footnotes.
key !== 'footnotes' &&
// Don't include private fields.
key.charAt( 0 ) !== '_' &&
// Only support string types.
(typeof entityMetaValues?.[ key ] === 'string' || typeof props.default === 'string')
) { // logic here }
There was a problem hiding this comment.
I agree getRegisteredPostMeta should be agnostic to bindings and in other case an object or an array could be a valid value. I want to explore if it should happen in the bindings hooks instead of just post meta.
And it's true that we can't just check against string because undefined is also a valid value. I'll spend some time today exploring the best way to handle this.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
While testing, I ran into the following error regarding 
This occurred with the following post meta registered in my |
| @@ -322,7 +322,7 @@ test.describe( 'Post Meta source', () => { | |||
| // Check the post meta fields are not visible. | |||
| const globalField = page | |||
| .getByRole( 'menuitemradio' ) | |||
| .filter( { hasText: 'text_custom_field' } ); | |||
| .filter( { hasText: 'Text custom field' } ); | |||
There was a problem hiding this comment.
I updated some tests here due to the label property now being supplied in packages/e2e-tests/plugins/block-bindings.php.
There was a problem hiding this comment.
Related to this, I believe text_custom_field in the following test should also be rendering as Text custom field, but it's not and I'm not sure why:
gutenberg/test/e2e/specs/editor/various/block-bindings/post-meta.spec.js
Lines 332 to 355 in 1bb2779
There was a problem hiding this comment.
It is because that test is part of the Custom Template tests, which doesn't support post meta yet (because we aren't able to get only the fields that apply to any post type).
Thinking about it, maybe it is better to create a new "String custom field"? We were using "text custom field" to test that the key is shown when there is no label, and that's a valid use case.
I’ve been triaging it and it is a problem of how the object custom field is registered. The default property is not valid. It needs to include a schema with the properties. And I am not sure if it is possible to define a default for an object type (I haven't been able to). I’ve updated the test example to reflect that. |
Cross-checking that would be the best for now. At least for the UI to select Post Meta filtring out post meta that doesn’t match the same type as the attribute would be ideal. |
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.
I still want to:
updateBlockAttributeshandle this.What?
Fix a bug where the Attributes panel breaks the block when there are fields with type "object" or "array" defined.
Why?
This is not the expected behavior.
How?
Restricting the type of fields that can be used.
Testing Instructions
I added a e2e test.