Blocks: Adding versioning and migrations to the Block API #3327

youknowriad · 2017-11-03T14:00:32Z

This PR does not solve all migrations paths. It only addresses the "automatic" migration on post edit strategy.

This is now ready for a first review. For testing purpose, I've upgraded the quote block to use cite instead of footer with a migration.

How does it work

This implements the parsing flow described here #2541 (comment)

All blocks are assumed to be in version 1 by default
You can increase the version of the block using a version property and the version is persisted in the block starting comment
If you do so, you'd more likely want to add a "migration" under the migrations property.
A migration consists of the old attributes, the old save function and the version to upgrade.
When parsing a block, if we detect that it's on an old version and an automatic migration exists, we apply it.

Notes

I noticed that saving the quote block with a cite and reloading the page adds some undesired <p> tags which causes the parsing to fail. I wonder if it's something related to autop that was not causing an issue when it was a footer tag instead.
I added a "version" to the block starting comment, but this revealed an issue with the Block_Attributes parser.

Todo:

Fix the autop behavior breaking cite
Fix the attributes as JSON parsing

Testing instructions

Paste this in to the code editor (old quote HTML)

<!-- wp:quote {"style":1} -->
<blockquote class="wp-block-quote blocks-quote-style-1">
    <p>The editor will endeavour to create a new page and post building experience that makes writing rich posts effortless, and has “blocks” to make it easy what today might take shortcodes, custom HTML, or “mystery meat” embed discovery.</p><footer>Matt Mullenweg, 2017</footer></blockquote>
<!-- /wp:quote -->

Click outside the editable area
You'll notice the block is "migrated" to the last version with cite

codecov · 2017-11-22T11:31:55Z

Codecov Report

Merging #3327 into master will increase coverage by 0.22%.
The diff coverage is 97.67%.

@@            Coverage Diff             @@
##           master    #3327      +/-   ##
==========================================
+ Coverage   36.93%   37.15%   +0.22%     
==========================================
  Files         268      268              
  Lines        6676     6701      +25     
  Branches     1203     1209       +6     
==========================================
+ Hits         2466     2490      +24     
- Misses       3557     3558       +1     
  Partials      653      653

Impacted Files	Coverage Δ
blocks/api/raw-handling/shortcode-converter.js	`52.38% <ø> (ø)`	⬆️
blocks/api/raw-handling/index.js	`84.21% <ø> (ø)`	⬆️
blocks/library/quote/index.js	`27.9% <100%> (+9.48%)`	⬆️
blocks/api/serializer.js	`98.03% <100%> (+0.03%)`	⬆️
blocks/api/parser.js	`97.1% <97.05%> (-0.9%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 300ca0a...545bf16. Read the comment docs.

youknowriad · 2017-11-22T11:42:23Z

blocks/api/post.pegjs

@@ -296,12 +352,19 @@ Block_Name_Part
  = $( [a-z][a-z0-9_-]* )

 Block_Attributes
-  = attrs:$("{" (!("}" WS+ """/"? "-->") .)* "}")
+  = attrs:$("{" (!("}") .)* "}")


This change was necessary. Without this, the parser was not parsing the blocks with attributes and version properly.

I'm aware this change is not good because it means the "JSON" can't contain "}" at all. Would love some help here @aduth @dmsnell

I'd strongly discourage the changes as they are because I think it overcomplicates the parser and it breaks JSON.

The nice thing about the system we were using is that it fits within normal HTML comments without breaking and only imposes the additional constraint that -- be escaped, which would be necessary regardless of JSON since we're already inside an HTML comment.

immediately I can imagine a few alternatives here:

put the version string before the attributes. this requires few changes and would be quite easy in the grammar spec. the version being prefixed would immediately fail when not present, so we can just add it in before the attributes as an optional value

store the version inside the attributes - no changes to the parser are even required.

It's worth noting too that the existing code is a massive optimization. We could support a full JSON parse here and not worry about eating all the input until the block header ends, but then we'd have to include a full JSON parser in this grammar. As-is, we do a fast scan to the end then feed in the text to the native JSON parser built in the browser or in PHP which will be magnitudes faster than if we hand-built one.

I tried something like

Block_Header = "" { /** <?php return array( 'blockName' => $blockName, 'attrs' => $attrs, 'version' => $version ? $version : 1, ); ?> **/ return { blockName: blockName, attrs: attrs, version: version }; }

Similar to what we had before, but it's not working and I'm not sure what I am doing wrong.

Ok! I was able to revert the JSON parsing change by inverting the version and the attributes.

What is problematic about storing the version in the attributes?

Nothing aside the "version" becomes a special attribute and we should guard against its usage in regular attributes.

What's problematic without storing it outside the attributes, since it's not an attribute semantically speaking?

Just to be clear, I'm not against this change.

It’s a special attribute for sure, though with a stunningly obvious meaning and one we could easily communicate about and guard against (for example, but stripping it before blocks get their attributes).

I’d say the cost is adding complexity to the parser where we already have a system for storing attributes. I’m not sure I’d argue that the version isn’t an attribute.

Of course it’s fine if this is one of those intrinsic things every block has and is duly different than attributes to store it elsewhere, though I’m guessing it only feels different because we already combined separate attributes in the HTML style into the JSON structure in order to be more terse and eliminate escaping-confusion.

How would this be different if attributes had the same kind of key-value pairs in the block header?

dmsnell · 2017-11-22T22:56:20Z

blocks/api/post.pegjs

-    /** <?php return $a; ?> **/
-    return a;
-  })? "-->"
+  = "<!--" WS+ header:Block_Header WS+ "-->"


This feels a bit obscure and I'm not sure what value it brings us to pull out the Block_Header here into its own rule. We're not saving much repetition, and we end up with more code even I think.

Is there a reason why we created this wrapper? The wrapper rule isn't even doing anything but opaquely passing everything through Block_Header and so it seems to make it harder to figure out what the rule is actually doing. I would hope that as much as is possible we could keep the structure of the rules close to the structure of the document being parsed; in this case maybe that means including some repetitive bits because at a glance we can see how it's supposed to be.

This is a factorization because the same thing is used in Block_Start and Block_Void

Oh yeah it’s obvious what it is; but it’s not clear how this is any better. I was simply saying that to me it appears to obscure the grammar and I don’t find that helpful.

Why is the factoring being done and how does it help someone reading or using it? If all it’s doing is removing one duplication of a few lines and it’s cost is a level of indirection that’s doing nothing then I don’t think that’s a very good bargain

Why is the factoring being done and how does it help someone reading or using it?

I do think it helps people reading and maintaing the grammar. In fact when I implemented the versioning, it was broken for void blocks at first. It's easy to miss the void blocks if you're not aware they exist.

I'd be interested in hearing some additional thoughts on that. To me it makes the grammar more confusing though I can understand where a view might be different than mine.

dmsnell · 2017-11-22T22:58:01Z

blocks/api/post.pegjs

+  {
+    /** <?php return (int) $v; ?> **/
+    return parseInt( v, 10 );
+  }


It looks like we're wanting to see something like v153840 here - is there a reason not to rely instead on an HTML-like attribute? seems like that could be less surprising to someone seeing it



With v1348 it looks like we have an attributes whose name is v1348 and whose value is true

No reason and preference here

dmsnell · 2017-11-22T23:03:06Z

blocks/api/parser.js

+	// as invalid, or future serialization attempt results in an error
+	block.originalContent = originalContent;
+
+	return block;


It kind of looks like this function is operating some kind of pipeline on the raw blocks: get a block from the parser, see if we have version stuff, see if we have fallback stuff, etc…

Since we're getting deep in the nesting do you think it might be time to consider splitting up that logic into isolated and more easily reasoned-about and testable pieces then turn this function into a composition of those steps?

I'm seeing the start of some kind of recursive operation with an accumulator passing things like [ block, arguments, shouldFallback ]

if { if { if { } else { } } else { // can you quickly tell me under what circumstances I will run? } } else { }

^^^ that's starting to get hard to follow 😉

Yes, I know, it's not easy to break actually. Just take a look at the flow chart linked above. The "reusable" pieces are already outside the function. Do you have a suggestion on how we can improve it? I'm all for it.

dmsnell

Thanks for working on the parser! I've left some recommendations and I hope they don't seem too severe; I would strongly encourage some iteration on this, thinking first of the goal and then if we can accomplish that in different ways.

Also, this isn't specific here, but these fixtures are adding a lot of noise to the PR. Would it be reasonable to undo all fixture changes until the PR is ready for final review so that we can focus on the actual changes we want to enact?

Good stuff!

youknowriad · 2017-11-23T08:51:18Z

Also, this isn't specific here, but these fixtures are adding a lot of noise to the PR.

I can understand this, I feel the same in several PRs, but I have to admit the fixture test helped me a lot while working on this PR.

mtias · 2017-11-23T14:28:21Z

@youknowriad I am reluctant about the approach of adding a version attribute to the comment for the purpose of supporting deprecated sourcing and migrating to current ones. This means eventually all blocks will have version number attributes, creating a convoluted presentation.

For the current use cases we have, I'd rather we looked at just handling the old attributes better. Example:

deprecatedAttributes: {
    caption: {
        type: 'array',
        source: 'children',
        selector: 'footer',
    },
}

Then the process would look like this:

Attempt to extract block attributes using the current attributes definition.
If it fails for an attribute, look at whether deprecatedAttributes and an attribute of the same name exists.
Use that definition to source the attribute.
On save, if we had used a legacyAttribute, consider some of the following:
- Don't invalidate a mismatch of raw vs save and just update the result.
- Show a message: "markup has been updated".
- Offer dialog to migrate.

Another way, suggested before by @mcsf could be:

attributes: {
    caption: {
        type: 'array',
        source: 'children',
        selector: 'cite',
        deprecated: [ { selector: 'footer' } ],
    },
}

This would allow a compact sequence of multiple deprecated values, used in order.

Are there other use cases that would really need a structured versioning system? Is there another approach that doesn't have to include the version in the comment?

youknowriad · 2017-11-23T14:42:46Z

I understand the concern but the proposed solution has flaws IMO:

If it fails for an attribute,

What does "fail" mean, the attribute is returned as "undefined"? this can be "valid". The matchers don't fail IMO.

On save, if we had used a legacyAttribute, consider some of the following:

We have no way to ensure the block has not been invalidated in this case.

Are there other use cases that would really need a structured versioning system?

What if the block is updated more than once? we'd need several deprecated attributes?

Is there another approach that doesn't have to include the version in the comment?

The only other approach is to recommend to people to not change their blocks and create blocks with different names. It's not sustainable for core blocks though.

As much as I understand the concerns, it also adds a bit of complexity to the parser, IMO a structured versioning system is the only reliable way to handle changes to existing blocks.

mtias · 2017-11-23T15:36:21Z

What does "fail" mean, the attribute is returned as "undefined"? this can be "valid". The matchers don't fail IMO.

I understand this, but it might be alright to do a secondary check for a deprecated attribute if something comes as undefined.

What if the block is updated more than once? we'd need several deprecated attributes?

Yes, the example with the array shows how that might work.

As much as I understand the concerns, it also adds a bit of complexity to the parser

What sort of complexity? I think the current state of this PR is also complex. :) What makes me pause is what versioning even means for something that is not yet a block until it is parsed. That is the nature of static blocks, we are basically defining an edit context that consumes from sourced attributes, where variations are really just other possible attributes to source from.

The only problem there is the validation approach we have in place, where really just one save function is appropriate. I think that has to be true, otherwise blocks will grow more complex with time.

Maybe we have a way of registering deprecated blocks like wp:deprecated/quote and that scales better, centralizing definitions and migrations without affecting the "current" version's footprint? (The way it would work is when a block fails validation, and there is a registered deprecated version, that block would take over and sort things out.)

youknowriad · 2017-11-27T09:49:47Z

Thanks folks for the reviews, I've followed @mtias's suggestion in #3665 which proves to be way simpler.

youknowriad self-assigned this Nov 3, 2017

youknowriad force-pushed the try/block-versioning branch 3 times, most recently from b354ba4 to 01478f0 Compare November 22, 2017 11:22

Blocks: Adding versioning and migrations to the Block API

ed66989

youknowriad force-pushed the try/block-versioning branch from 01478f0 to ed66989 Compare November 22, 2017 11:31

youknowriad commented Nov 22, 2017

View reviewed changes

youknowriad requested a review from aduth November 22, 2017 12:30

dmsnell reviewed Nov 22, 2017

View reviewed changes

dmsnell requested changes Nov 22, 2017

View reviewed changes

youknowriad requested a review from mtias November 23, 2017 08:48

Parser: Invert the version and the attributes in the block's comment

545bf16

youknowriad force-pushed the try/block-versioning branch from f27365e to 545bf16 Compare November 23, 2017 08:49

youknowriad mentioned this pull request Nov 27, 2017

Framework: Handle and upgrade deprecated blocks #3665

Merged

youknowriad closed this Nov 27, 2017

youknowriad deleted the try/block-versioning branch November 27, 2017 09:49

efuller mentioned this pull request Mar 15, 2018

Add Deprecated Function for Existing Blocks WebDevStudios/wds-blocks#24

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Adding versioning and migrations to the Block API #3327

Blocks: Adding versioning and migrations to the Block API #3327

youknowriad commented Nov 3, 2017 •

edited

Loading

codecov bot commented Nov 22, 2017 •

edited

Loading

youknowriad Nov 22, 2017

dmsnell Nov 22, 2017

dmsnell Nov 22, 2017

youknowriad Nov 23, 2017

youknowriad Nov 23, 2017 •

edited

Loading

dmsnell Nov 23, 2017

youknowriad Nov 23, 2017

dmsnell Nov 23, 2017

dmsnell Nov 22, 2017

youknowriad Nov 23, 2017

dmsnell Nov 23, 2017

youknowriad Nov 23, 2017 •

edited

Loading

dmsnell Nov 23, 2017

dmsnell Nov 22, 2017

youknowriad Nov 23, 2017

dmsnell Nov 22, 2017 •

edited

Loading

youknowriad Nov 23, 2017

dmsnell left a comment

youknowriad commented Nov 23, 2017

mtias commented Nov 23, 2017

youknowriad commented Nov 23, 2017

mtias commented Nov 23, 2017 •

edited

Loading

youknowriad commented Nov 27, 2017

Blocks: Adding versioning and migrations to the Block API #3327

Blocks: Adding versioning and migrations to the Block API #3327

Conversation

youknowriad commented Nov 3, 2017 • edited Loading

codecov bot commented Nov 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

youknowriad commented Nov 23, 2017

mtias commented Nov 23, 2017

youknowriad commented Nov 23, 2017

mtias commented Nov 23, 2017 • edited Loading

youknowriad commented Nov 27, 2017

youknowriad commented Nov 3, 2017 •

edited

Loading

codecov bot commented Nov 22, 2017 •

edited

Loading

youknowriad Nov 23, 2017 •

edited

Loading

youknowriad Nov 23, 2017 •

edited

Loading

dmsnell Nov 22, 2017 •

edited

Loading

mtias commented Nov 23, 2017 •

edited

Loading