Gallerys ids are saved as numbers

[SergioEstevao]

Description

Changed the JSON schema for the gallery blocks to ensure that the ids array is always made with numbers.

How has this been tested?

• Create a gallery block with images from WP media library
• Switch to code and see that the ids are ints
• Save & Close post
• Reopen post
• Select a tile and move left or right
• Switch to code view and see that the id are still numbers
• Edit the HTML and change the ids to strings
• Switch to Visual mode and see that all is ok
• Switch to HTML mode and see if the block was saved with numbers.

Screenshots

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[aduth]

Changed the JSON schema for the gallery blocks to ensure that the ids array is always made with numbers.

Can you explain why? What's the context here?

The general idea seems to make sense. There's a lot of code here to make that happen though, which is pretty unfortunate.

[SergioEstevao]

Can you explain why? What's the context here?

When implementing support for async media publishing on the media apps, we have scenarios were the media is uploaded in the background when Gutenberg editor is not even open.

When that happens we have some text processors that find the block code and update the media information.

When doing that work we found this situation where ids were being saved sometimes like string and another times like ints.

We coded around it, but I feel this is something that should be consistent on the block saving code.

[SergioEstevao]

@aduth ready for another look.

[koke]

I'm still getting strings, testing on web:

1. Create a new post and add a gallery
2. View HTML, IDs are numbers
3. Save and reopen, IDs are numbers
4. Reorder some of the images, IDs are strings
5. Save and reopen, IDs are numbers

So it seems that at least the deprecation path is working and it's recovering automatically from the strings case, but it's still storing image IDs as strings internally, since they get replaced when the images are reordered.

I don't understand the parser code 100% but it seems to me that attributes extracted from HTML attributes are always strings, regardless of the type specified. Maybe we need some extra code in save.js to ensure that those IDs are always numeric, but I'd appreciate a second opinion on that.

[aduth]

Is there any opportunity for code reuse in any of these properties or functions? Merging attributes using Object.assign / spread? Reference a common save function?

This is a lot of code to coerce an array to a typed array.

[SergioEstevao]

@aduth I considerer shared code with the current version of save, but then I saw this comment on the block deprecations documentation:

It’s important to note that attributes, supports, and save are not automatically inherited from the current version, since they can impact parsing and serialization of a block, so they must be defined on the deprecated object in order to be processed during a migration.

Is this still a valid concern? Or I can share code without problems with the current version of save?

[ZebulanStanphill]

The problem is that if you inherit from the current attributes, the deprecations may work now but not later if more attributes are added to the block. If you add an optional thingy attribute to the current block, and your deprecations inherit the entire attributes object from the current implementation, then all the deprecations will stop working. If you change an existing attribute, you have to remember to modify every single deprecation to stop inheriting from the current implementation.

Hardcoding the deprecation definitions, although verbose, is a more robust solution. I don't really see it as much of problem since the deprecations are now usually put in a separate file from the rest of the block data.

[talldan]

Yeah, I prefer the longer safer option. Though it's generally ok if there are tests in place.

Which is something I think should be considered for this PR, ideally a core__gallery__deprecated-4.html fixture would be added in this folder:
https://github.com/WordPress/gutenberg/tree/master/packages/e2e-tests/fixtures/blocks

The way I generally do this is:

1. Check out a branch with the right version of the block for the deprecation (so the gallery block from master)
2. Insert it into a post
3. Copy the markup using the Code Editor view
4. Switch back to this branch
5. Paste it into a new html file core__gallery__deprecated-4.html in the packages/e2e-tests/fixtures/blocks folder.
6. Generate the other fixture files by running GENERATE_MISSING_FIXTURES=y npm run test-unit test/integration/full-content/full-content.test.js
7. Commit and push.