Ensure API returns empty attributes to maintain typing contract integrity

[Zamfi99]

Description

The current implementation of the content parser doesn't return empty attributes, which is a deviation from the expected behavior. This inconsistency can potentially break the typing contract, causing downstream issues for applications relying on this API.

Proposed Solution

This pull request addresses the issue by modifying the content parser logic to ensure that empty attributes are included in the response.

Steps to Test

1. Check out PR.
2. Run composer run test.

[alecgeatches]

@Zamfi99 Thank you for this PR! I think I understand your reasoning, but I'm not sure if we can add this feature to the Block Data API. There are a couple of reasons:

1. It's not clear what default value should be returned when none is provided. When we source an attribute, we provide the defined default value for that attribute where available. If one isn't provided, I don't think an empty string will always be the correct default. From the WordPress block attributes documentation:

   Most attributes from markup will be of type string. Numeric attributes in HTML are still stored as strings, and are not converted automatically.

   The only exception is when checking for the existence of an attribute (for example, the disabled attribute on a button). In that case type boolean can be used and the stored value will be a boolean.

   Example: Extract the disabled attribute from a button found in the block’s markup.

   Saved content:

   <div>
       Block Content
   
       <button type="button" disabled>Button</button>
   </div>
   

   Attribute definition:

   {
       disabled: {
           type: 'boolean',
           source: 'attribute',
           selector: 'button',
           attribute: 'disabled',
       }
   }
   

   Attribute available in the block:

   { "disabled": true }
   

   In the case above, disabled is a sourced attribute without a default value. If we were providing a value, an empty string '' doesn't make sense for a boolean-typed attribute. And it'll depend on the attribute context whether false or true makes sense as a default.

2. This is a breaking change for both the GraphQL and REST API. For example, here's the current trunk output for a sample image block, using the REST API as an example for brevity:

   {
       "name": "core/image",
       "attributes": {
           "id": 8,
           "sizeSlug": "large",
           "linkDestination": "none",
           "url": "http:/my.site/wp-content/uploads/image.jpg",
           "alt": "",
           "width": 1024,
           "height": 680
       }
   }
   

   and this is the result from this current branch:

   {
       "name": "core/image",
       "attributes": {
           "id": 8,
           "sizeSlug": "large",
           "linkDestination": "none",
           "url": "http://my.site/wp-content/uploads/image.jpg",
           "alt": "",
           "caption": "",
           "title": "",
           "href": "",
           "rel": "",
           "linkClass": "",
           "linkTarget": "",
           "width": 1024,
           "height": 680
       }
   }
   

   REST or GraphQL code that relies on the old behavior will now receive different data. We're willing to make breaking changes when we need to, but this may not be one of those cases.

In summary, I think that we don't want to start providing our own default sourced values. Gutenberg has a mechanism for providing defaults via "default":, and attributes that do not provide a default value get no value, by contract. If we start providing defaults where none are specified, we'll also need to make assumptions on unknown block attributes. I also just merged https://github.com/Automattic/vip-block-data-api/pull/70 which provides some tests around our handling of default attributes to ensure consistency. The main thing is that we now ensure that values with no default are not returned.

For your problem on the GraphQL side, I think it may be best to keep contract integrity by using a nullable type or some equivalent in your consumer code. There may be an alternative solution we can do that only affects the GraphQL API, or maybe a filter of some sort. Let us know if there's something else that would be helpful here or something we've missed. Thank you!

[Zamfi99]

Hello @alecgeatches, It would be great if we can add a filter that will allow us to do that! Let me know if you want me to update this PR in order to achieve that.

[alecgeatches]

@Zamfi99 My apologies for not getting back to you sooner here.

It would be great if we can add a filter that will allow us to do that! Let me know if you want me to update this PR in order to achieve that.

I'd appreciate that! If you can provide a filter to turn on empty attribute returns I think we'd be fine with adding that as a non-default behavior. Thank you!

[ingeniumed]

I'm closing off this PR, feel free to put up a new PR with the proposed change by @alecgeatches.