Opened 8 months ago
Last modified 5 weeks ago
#63325 reopened defect (bug)
Regression in saving posts with Block Editor in 6.8
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | Future Release | Priority: | normal |
| Severity: | normal | Version: | 6.8 |
| Component: | Editor | Keywords: | has-patch has-unit-tests needs-test-info |
| Focuses: | Cc: |
Description
I believe this to be an issue on the save portion of the rest API. When saving the raw post data in the visual editor, we often have empty JS/JSON objects in our data; when this is saved, they are being transformed into arrays. I believe this is being done via the save post rest end point as checking the data after saving a post shows the post content having arrays. We have also noticed this before when saving post data when the user saving the post is not an admin. With WP 6.8 this also happening with users that have admin privilege.
Example:
<!-- 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 -->
Which on save is being transformed into:
<!-- wp:nectar-blocks/button {"blockId":"block-RX3iloq4aK","bgColor":{"desktop":{"type":"solid","solidValue":"#af2c2c","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 -->
If this is the expected behavior, is there a way to disable it, or otherwise filter out this functionality? This is fundamentally changing the data we send to the backend and then expect to be returned.
Change History (37)
#1
@
8 months ago
- Milestone Awaiting Review deleted
- Resolution set to reported-upstream
- Status changed from new to closed
This ticket was mentioned in Slack in #core by margolisj. View the logs.
8 months ago
This ticket was mentioned in PR #8735 on WordPress/wordpress-develop by @margolisj.
8 months ago
#3
- Keywords has-patch has-unit-tests added
#4
@
8 months ago
@wildworks Wouldn't it make sense to keep the ticket open? The fix will likely need to be made in Core, and it might help to have the ticket around so folks are aware in case it's decided that it should go into 6.8.x...
@Bernhard Reiter commented on PR #8735:
8 months ago
#5
Thank you for working on a fix for this!
Since I apparently introduced this in https://core.trac.wordpress.org/changeset/59523, it might be sufficient to fix apply_block_hooks_to_content to respect the array or object nature of block attributes -- especially since AFAICS it might be really hard to fix serialize_block_attributes to return the correct kind of attribute even if it doesn't have sufficient context.
I'll see if I can figure out a fix for apply_block_hooks_to_content.
#6
@
8 months ago
- Milestone set to 6.8.1
- Resolution reported-upstream deleted
- Status changed from closed to reopened
Wouldn't it make sense to keep the ticket open?
That's right, I initially closed this because I thought this issue was directly related to the Block Hooks API, but it actually looks like it needs to be fixed in core, so I'm reopening it.
#7
@
8 months ago
- Milestone changed from 6.8.1 to 6.8.2
With 6.8.1 RC1 due out in a few hours, I'm going to punt this as there's still more work required.
This ticket was mentioned in PR #8789 on WordPress/wordpress-develop by @Bernhard Reiter.
7 months ago
#8
In serialize_attributes, if the value of an attribute is an empty array(), 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 empty array() 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) and serializeAttributes (JS), as the PHP version needs to no 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. into get_comment_delimited_block_content. This should still be sufficient for all practical purposes; it is also currently the only callsite of serialize_attributes in the entire Core codebase.
Trac ticket: https://core.trac.wordpress.org/ticket/63325
Gutenberg issue: https://github.com/WordPress/gutenberg/issues/69959
Prior art: https://github.com/WordPress/wordpress-develop/pull/8735
@Bernhard Reiter commented on PR #8735:
7 months ago
#9
Right, so this is a tricky one; I haven’t found a way to tackle it at apply_block_hooks_to_content level.
Here's an attempt based on inferring the attribute type from the block definition: https://github.com/WordPress/wordpress-develop/pull/8789
7 months ago
#10
Alternatively, we can move the logic I’m introducing here one level up, i.e. into get_comment_delimited_block_content. This should still be sufficient for all practical purposes; it is also currently the only callsite of serialize_attributes in the entire Core codebase.
That sounds more compelling to me as this would leave serialize_attributes with the old signature, helping to avoid adding a bit confusing behavior. I was looking at the code, and it doesn't look it would be straightforward to map an empty array to an empty object during block parsing. It feels like that would be the best place to ensure proper type recognition.
@Bernhard Reiter commented on PR #8789:
7 months ago
#11
BTW this still doesn't solve the problem for empty arrays that are nested inside objects, which was the original example in https://github.com/WordPress/gutenberg/issues/69959, e.g.
<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>
Note that bgColor is an object; its tablet, mobile, and hover properties are all empty objects ({}). These will currently get transformed into []s, as block.json doesn't contain "full" schema information about object-type attributes.
The only piece of information that we _might_ be able to use if the default, e.g. https://github.com/WordPress/wordpress-develop/blob/3f9317c2aa2ba650c0d930acaca1bb840c95a6da/src/wp-includes/blocks/query/block.json#L14-L32
Might be material for a follow-up, though, as it gets us into slightly more "heuristic" territory.
@wildworks commented on PR #8789:
7 months ago
#12
This should still be sufficient for all practical purposes; it is also currently the only callsite of
serialize_attributesin the entire Core codebase.
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.
@getsyash commented on PR #8789:
7 months ago
#13
Thanks for tackling this — it's a subtle edge case with real consequences, especially for block attributes that depend on accurate type preservation.
I agree that introducing $block_name into serialize_attributes() complicates its previously low-level, stateless nature. Moving this logic into get_comment_delimited_block_content() seems like a cleaner solution and keeps serialize_attributes() generic and reusable.
Also appreciate the note about nested empty arrays — definitely sounds like it warrants a follow-up, maybe with enhancements to how block schemas are defined or parsed (especially from block.json).
7 months ago
#14
BTW this still doesn't solve the problem for empty arrays that are nested inside objects, which was the original example in https://github.com/WordPress/gutenberg/issues/69959, e.g.
One way to solve it would be to introduce another field similar to enum that would allow defining the JSON schema describing the shape of the objects and arrays. In fact, it wouldn't differ that much from what enum offers today, but with greater flexibility.
7 months ago
#15
The root cause lies in how block attributes gets decoded from the serialized JSON:
In effect, empty arrays and empty objects become indistinguishable in PHP.
The same case in JavaScript gets handled differently:
Example:
@Bernhard Reiter commented on PR #8789:
7 months ago
#16
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.
One way to solve it would be to introduce another field similar to
enumthat would allow defining the JSON schema describing the shape of the objects and arrays. In fact, it wouldn't differ that much from whatenumoffers today, but with greater flexibility.
Indeed. But that sounds like more of a longer-term solution to me, as it'll require both updating the block.json schema (to allow a JSON schema for its attribute types); Core blocks to be updated to specify that attribute type schema where appropriate; and 3rd party block authors to adopt it as well.
7 months ago
#17
What happens if we skip serializing empty arrays? I’m unnerved for the same reasons you are about reading the block registry on serialization:
- it doesn’t solve this problem; it only helps in some cases where the block attribute definitions are available and where the attributes are at the top-level
- it couples a behavior that works universally with one that’s now highly dependent on the runtime environment, because my system will serialize blocks differently than yours and my system might even serialize differently based on what plugins are activated, meaning this is different now than it was an hour ago
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 $block['attrs']['settings'] = []
// decoding attributes on parse
foreach ( walk_parsed_attributes() as $name => &$value ) {
if ( $value instance stdClass ) {
$value['__wp_meta__is_js_map'] = true;
}
}
// serializing
if ( is_array( $value ) && isset( $value['__wp_meta__is_js_map'] ) ) {
unset( $value['__wp_meta__is_js_map'] );
$value = (stdClass) $value;
}
@Bernhard Reiter commented on PR #8789:
7 months ago
#18
@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.)
7 months ago
#19
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.
7 months ago
#20
@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 json_decode() in PHP because we can tell json_decode() to return stdClass for objects and array for arrays. so if we use json_decode( $json, false ) we now retrieve a nested object hierarchy which we can convert into numeric and associative arrays. it does involve some overhead in the conversion, but I would think not much
on serialize we erase any residual of this meta-typing
---
care of mlx-community/Qwen3-32B
/**
* 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.
7 months ago
#21
I understand the idea now. The changes would have to be applied on the block parsing level:
Namely, the second parameter would have to get updated to false to ensure that empty objects are represented as stdClass. One alternative could wrapping stdClass with an instance of the class that implements array access. An illustration of the idea:
<?php class BlockAttributes extends stdClass implements ArrayAccess { function __construct( $metadata ) { $data = json_decode( $metadata ); foreach ( $data as $key => $value ) { $this->$key = $value; } } public function offsetSet($offset, $value): void { $this->$offset = $value; } public function offsetExists($offset): bool { return isset($this->$offset); } public function offsetUnset($offset): void { unset($this->$offset); } public function offsetGet($offset): mixed { return isset($this->$offset) ? $this->$offset : null; } }
7 months ago
#22
wrapping stdClass with an instance of the class that implements array access
I tried doing just that in #5705 but ran into troubles:
- user-space code is sometimes setting the attributes to
array(), which wipes out any structure we intend on adding to the block attributes (and this is left unresolved with the discussion here because it will remain an issue) - there were seeming language-level issues with code accessing or possibly only when setting nested attributes where the
ArrayAccessseemed to me to not do what the documentation claims, like$block['attrs']['settings']['thing'] = 'bloated'
if we could get this to work reliably I would love that, because it would imply a path forward for moving to a lazy-by-default block parser, but I worry. would love to see that my worries are misplaced.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
6 months ago
#24
@
6 months ago
This ticket was reviewed during today's bug scrub.
As this is still in an ongoing discussion, we'll keep it in the milestone. Just adding a comment to make sure it's still on your plate for 6.8.2. Ideally a patch would be committed in the next couple weeks.
Also it would be nice to add an owner to this ticket :)
This ticket was mentioned in PR #9020 on WordPress/wordpress-develop by @jorbin.
6 months ago
#25
Run the new tests added in https://github.com/WordPress/wordpress-develop/pull/8789/ on 6.7 branch to ensure backwards compatability is restored from 6.7 -> 6.8
Trac ticket: https://core.trac.wordpress.org/ticket/63325
#26
@
6 months ago
It seems as though while this PR provides a way to force a specific type on save, it is still a change from the pre 6.8 behavior
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
6 months ago
This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.
5 months ago
#30
@
5 months ago
- Keywords needs-test-info added; needs-testing removed
Combined Bug Reproduction and Patch Test Report
Description
🟠 This report validates that the issue can be reproduced but the patch is not working as expected
Patch tested: https://github.com/WordPress/wordpress-develop/pull/8789.diff
Environment
- WordPress: 6.9-alpha-60093-src
- PHP: 8.2.28
- Server: nginx/1.29.0
- Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
- Browser: Chrome 138.0.0.0
- OS: Windows 10/11
- Theme: Twenty Twenty-Five 1.2
- Plugins:
- Test Reports 1.2.0
Reproduction Steps
- Use the code provided by OP
- 🐞 There is an unexpected array transformation
Expected Results
- JSON empty values are kept as-is
Actual Results with Patch
- ❌ Values are still being stored as arrays
Additional Notes
- I've been reading over the report, but I still have some elements unclear. I have not thoroughly read the proposed code in PR 8789, so I wonder if the code is simply not finished yet or if I'm missing something.
cc @bernhard-reiter
- Also @jorbin your PR is an incremental code over PR 8789?
This ticket was mentioned in Slack in #core by zunaid321. View the logs.
5 months ago
#32
@
5 months ago
This ticket was brought up in a recent bug scrub.
Any update or comments on the report that was posted above will be highly appreciated to carry the ticket to a closure.
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
5 months ago
#34
@
5 months ago
- Milestone changed from 6.8.2 to 6.9
As per today's bug scrub: this ticket is moved to milestone 6.9 as 6.8.2 RC1 is tomorrow.
Thanks for the report.
I found the same issue is submitted on the Gutenberg repo: https://github.com/WordPress/gutenberg/issues/69959
I'm going to close this ticket as "reported-upstream".