Block Fields: Remove normalization code and tidy up #74532
Merged
+96
−343
Conversation
This file contains hidden or 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
github-actions
bot
added
[Package] Block library
talldan
commented
Jan 12, 2026
Comment on lines
-151
to
-169
| if ( fieldDef?.mapping ) { | ||
| Object.keys( fieldDef.mapping ).forEach( | ||
| ( key ) => { | ||
| if ( key === 'href' || key === 'url' ) { | ||
| updateValue[ key ] = | ||
| updatedAttrs.url; | ||
| } else if ( key === 'rel' ) { | ||
| updateValue[ key ] = | ||
| updatedAttrs.rel; | ||
| } else if ( | ||
| key === 'target' || | ||
| key === 'linkTarget' | ||
| ) { | ||
| updateValue[ key ] = | ||
| updatedAttrs.linkTarget; | ||
| } | ||
| } | ||
| ); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This code isn't required as the setValues function already does this by iterating over mapping.
|
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. |
talldan
commented
Jan 12, 2026
Comment on lines
-180
to
-188
| if ( | ||
| key === 'href' || | ||
| key === 'url' || | ||
| key === 'rel' || | ||
| key === 'target' || | ||
| key === 'linkTarget' | ||
| ) { | ||
| removeValue[ key ] = undefined; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The if statement isn't needed as mapping should already have the correct key names.
talldan
commented
Jan 12, 2026
| } ); | ||
| onChange( mappedChanges ); | ||
| }; | ||
| const { allowedTypes = [], multiple = false } = fieldDef.args || {}; |
Contributor
Author
There was a problem hiding this comment.
Fixes the allowedTypes by reading them from args instead of config.
Member
There was a problem hiding this comment.
Does media-thumbnail also need to read from fieldDef.args like we're doing here?
allowedTypes is empty otherwise.
Contributor
Author
There was a problem hiding this comment.
Good catch, the wrong icon was being shown for videos and other types. Pushed a fix for that 👍
talldan
commented
Jan 12, 2026
Comment on lines
-155
to
-163
| if ( | ||
| key === 'id' || | ||
| key === 'src' || | ||
| key === 'url' | ||
| ) { | ||
| resetValue[ key ] = undefined; | ||
| } else if ( key === 'caption' || key === 'alt' ) { | ||
| resetValue[ key ] = ''; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Removing this doesn't seem to cause any issues in my testing. Instead every key is now set to undefined on a reset.
|
Size Change: -606 B (-0.02%) Total Size: 3.05 MB
ℹ️ View Unchanged
|
talldan
commented
Jan 12, 2026
|
|
||
| // Turn off featured image when resetting (only if it's in the mapping) | ||
| if ( hasFeaturedImageSupport ) { | ||
| resetValue.featuredImage = false; |
Contributor
Author
There was a problem hiding this comment.
featuredImage also gets set back to undefined. Blocks already implement a default of false, so no need to set it to false.
talldan
commented
Jan 12, 2026
Comment on lines
+175
to
+178
| const newValue = { | ||
| ...selectedMedia, | ||
| mediaType: selectedMedia.media_type, | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
I've deleted a lot of the code that was previously here. Now uses keys and values in selectedMedia rather than trying to change them. The field.setValue implementation will map back to the keys that the block expects.
That is, apart from media_type - field ids are usually camel cased, so I've added a mediaType property here to support that.
talldan
force-pushed
the
remove/block-fields-normalization
branch
from
34d4364 to
9b442f5
Compare
talldan
commented
Jan 12, 2026
Comment on lines
+112
to
+118
| // If the field defines a `mapping`, then custom `getValue` and `setValue` | ||
| // implementations are provided. | ||
| // These functions map from the inconsistent attribute keys found on blocks | ||
| // to consistent keys that the field can use internally (and back again). | ||
| // When `mapping` isn't provided, we can use the field API's default | ||
| // implementation of these functions. | ||
| if ( fieldDef.mapping ) { |
Contributor
Author
There was a problem hiding this comment.
Only add getValue / setValue when needed now, which is when there's a mapping.
Also added a comment to explain this.
talldan
commented
Jan 12, 2026
Comment on lines
+158
to
+162
| if ( ! blockTypeFields?.length ) { | ||
| // TODO - we might still want to show a placeholder for blocks with no fields. | ||
| // for example, a way to select the block. | ||
| return null; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Moved this early return a little bit earlier.
ramonjd
approved these changes
Jan 13, 2026
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
Nice clean up @talldan
allowedTypes and multiple were not being handled correctly by media fields (it always defaulted to image).
Cover block backgroundType was not being correctly set.
I checked both these values and they appear to be working as described.
Only left a couple of comments - I think the MediaThumbnail needs to reference field.args, and I wasn't sure about the varying object props passed tosetValue and getValue
Tentative ✅ if you need to change that small thing before merged. It's behind the experiment as well.
🚀
| } ); | ||
| onChange( mappedChanges ); | ||
| }; | ||
| const { allowedTypes = [], multiple = false } = fieldDef.args || {}; |
Member
There was a problem hiding this comment.
Does media-thumbnail also need to read from fieldDef.args like we're doing here?
allowedTypes is empty otherwise.
| updateAttributes( updateValue ); | ||
| onChange( | ||
| field.setValue( { | ||
| item: data, |
Member
There was a problem hiding this comment.
Looking at field.setValue = ( { value } ) => { in packages/block-editor/src/hooks/block-fields/index.js above... does item need to passed?
It's done in several places, so I assume so and (?) but just checking since the implementation appears different to getValue.
Contributor
Author
There was a problem hiding this comment.
I kept it as the type requires it:
Though it's not TS code, it still feels like good practice.
andrewserong
approved these changes
Jan 13, 2026
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?
This is a fairly substantial code quality PR for Block Fields, based on some discoveries made while working on a more experimental PR - #74435.
Also fixes a couple of small issues, so I've labelled it a bug fix.
Let me know if you'd like to break it up and I can try, but some of the changes are based on some of the other changes, so I found it easier to roll it into one PR.
Why?
The goal of this PR is to move a few steps closer to the regular DataForm API and remove any special code.
How?
Removals:
normalizeMediaValue,denormalizeMediaValue,normalizeLinkValue,denormalizeLinkValue- I found these function are largely doubling up work that was already being done (infield.setValueandfield.getValue), so they're removed.hideLabelFromVision- As discovered in Block Fields: show all form fields by default #74486, this property doesn't do anything, so it's now removed.field.config- I found that this property also doesn't do anything, the config is passed through viacreateConfiguredControl.config.defaultValues- I've removed all handling ofdefaultValues. I've realized that it's much easier to unset attribute values usingundefinedinstead, the block itself will then pass its default. 💡Small changes:
field.getValue/field.setValue- these functions can be simplified by only setting them on thefieldobject whenmappingexists. Whenmappingdoesn't exist we can use the default implementation that the fields api provides.createConfiguredControl- I've changed the function signature slightly, I found it makes it a little easier to read.mappingwithin the controls as possible.mappingis a special thing that was invented for Block Fields. The only place it's needed currently is for resetting links or media, since we need to know which properties to set back toundefined.onChange( field.setValue( // ... ) )inline. This matches the style used by fields in the dataforms package, so the idea is to make it a closer implementation.Fixes:
allowedTypesandmultiplewere not being handled correctly bymediafields (it always defaulted to image).backgroundTypewas not being correctly set.Testing Instructions