-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Serialize: Respect attribute type for empty arrays #8789
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
base: trunk
Are you sure you want to change the base?
Serialize: Respect attribute type for empty arrays #8789
Conversation
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
src/wp-includes/blocks.php
Outdated
// An empty `array()` is encoded as `[]` in JSON. However, it's possible | ||
// that the attribute type is really an object (associative array in PHP), | ||
// so we need to check for that. | ||
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block_name ); |
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.
Nit: It's probably better to move this outside of the loop. There's no reason to get a block type instance on each iteration.
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.
Ohh that's right 😅
The reason I put it there was to avoid the block type lookup altogether if there was no empty array at all. But we can probably just cache it after it's first been looked up.
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.
The registry is already working like a memory cache, right? The lookup shouldn't be a problem.
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.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
That sounds more compelling to me as this would leave |
BTW this still doesn't solve the problem for empty arrays that are nested inside objects, which was the original example in WordPress/gutenberg#69959, e.g. <!-- wp:nectar-blocks/button {"blockId":"block-RX3iloq4aK","bgColor":{"desktop":{"type":"solid","solidValue":"#2c80af","gradientValue":""},"tablet":{},"mobile":{},"hover":{}}} -->
<div class="wp-block-nectar-blocks-button nectar-blocks-button nectar-font-label" id="block-RX3iloq4aK"><a href="#" class="nectar__link nectar-blocks-button__inner"><span class="nectar-blocks-button__text"></span></a></div>
<!-- /wp:nectar-blocks/button --> Note that The only piece of information that we might be able to use is the wordpress-develop/src/wp-includes/blocks/query/block.json Lines 14 to 32 in 3f9317c
(In this example, we can assume that the Might be material for a follow-up, though, as it gets us into slightly more "heuristic" territory. Also, ironically, this is plagued by the same problem as the original bug: If the default is a |
If I understand correctly, if consumers do not specify the optional second argument, the return value will be the same as before, right? Because this function seems to be used by some plugins. |
Thanks for tackling this — it's a subtle edge case with real consequences, especially for block attributes that depend on accurate type preservation. |
One way to solve it would be to introduce another field similar to |
Indeed. But that sounds like more of a longer-term solution to me, as it'll require both updating the |
What happens if we skip serializing empty arrays? I’m unnerved for the same reasons you are about reading the block registry on serialization:
I’d be interested in approaching this from the other side: let’s declare a hidden attribute on all parsed map types and use the presence of that attribute upon serialization. Though, this is still incomplete because people will be running things like
|
@dmsnell Interesting idea 🤔 I was trying to find a way to add that sort of meta information (i.e. array or object?) but didn't think of that. I like it, it's pretty straight-forward! I'll give that a try 👍 (Not sure when I'll get around to; I'm about to wrap up my week and will be pretty busy next week.) |
Very interesting idea. Unless I misunderstood what you proposed, my only concern is that the problem exists at the JSON parsing level in PHP, so this meta information would have to be serialized in HTML on the client. In effect, it would also need to get removed during parsing on the client before passing the data to the block editor. It might require very detailed testing to ensure it doesn't break anything when this special property leaks somewhere. |
@gziolo the JS side doesn’t have this problem because it does differentiate between empty arrays and empty objects. In fact, we also don’t have this problem when calling on serialize we erase any residual of this meta-typing care of /**
* Recursively converts a nested stdClass object into an associative array.
* Every stdClass object converted into an array will include a "__js_map" => true
* marker key to indicate that conversion took place.
*
* @param mixed $data The input data structure which may include:
* - stdClass objects
* - arrays
* - scalars (string, bool, null, etc.)
*
* @return mixed The transformed structure with stdClass objects converted into arrays
*/
function convertToAssociativeArray($data) {
if (is_object($data) && get_class($data) === 'stdClass') {
// Initialize result array
$arrayResult = [];
// Recursively convert each property of the object
foreach ($data as $key => $value) {
$arrayResult[$key] = convertToAssociativeArray($value);
}
// Add the "__js_map" marker after processing all properties
$arrayResult['__js_map'] = true;
return $arrayResult;
}
if (is_array($data)) {
// Recursively process each element of the array
$result = [];
foreach ($data as $key => $value) {
$result[$key] = convertToAssociativeArray($value);
}
return $result;
}
// Return scalars, nulls, etc., unchanged
return $data;
} $obj = new stdClass();
$obj->name = "Alice";
$obj->age = 30;
$obj->tags = ['developer', 'PHP'];
$obj->metadata = new stdClass();
$obj->metadata->role = "Senior";
$obj->metadata->active = true;
$result = convertToAssociativeArray($obj);
print_r($result); Array
(
[name] => Alice
[age] => 30
[tags] => Array
(
[0] => developer
[1] => PHP
)
[metadata] => Array
(
[role] => Senior
[active] => 1
[__js_map] => true
)
[__js_map] => true
) we would ideally try to think of a non-recursive version, but this would be okay with better naming. |
In
serialize_attributes
, if the value of an attribute is an emptyarray()
, it will always be JSON-encoded into[]
even though it could be an associative array (which should be encoded into{}
) — but that distinction just doesn’t exist for empty arrays in PHP. In other words, PHP simply doesn’t have enough (meta) information to know what it should encode an emptyarray()
into.So our best bet is to infer the attribute type from the block definition. This isn't great, since
serialize_attributes
is a low-level function that was previously able to operate without any knowledge about higher-level block semantics, but alas.Note that this also violates function signature parity of
serialize_attributes
(PHP) andserializeAttributes
(JS), as the PHP version needs to know the block name in order to look up its block definition. Maybe the violation is permissible here, but I’d like to hear @dmsnell’s optionion before proceeding. Alternatively, we can move the logic I’m introducing here one level up, i.e. intoget_comment_delimited_block_content
. This should still be sufficient for all practical purposes; it is also currently the only callsite ofserialize_attributes
in the entire Core codebase.Trac ticket: https://core.trac.wordpress.org/ticket/63325
Gutenberg issue: WordPress/gutenberg#69959
Prior art: #8735 h/t @margolisj
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.