Opened 6 years ago
Closed 4 years ago
#47375 closed enhancement (worksforme)
Blocks API: Add server-side `serialize_block()`
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 5.3 |
Component: | Editor | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
We have the ability to parse_blocks()
on the server and transform serialized block content in HTML form into a list of block objects or nodes. We lack the ability to transform in the opposite direction.
serialize_block()
takes the output from parse_blocks()
and returns an HTML string which would produce the same content as at the beginning of the process. in code terms…
<?php $post_content === implode( "\n\n", array_map( 'serialize_block', parse_blocks( $post_content ) ) );
An initial use-case I have found is the desire to transform the saved contents of $post_content
from a block-structural point of view. I want to "extract" the inner contents of a block from its surrounding block comment delimiters without touching the other parts of $post_content
.
Another use-case would be any similar structural changes. Consider the case where we want to update a post and remove all align: left
attributes from the comment delimiters…
<?php $blocks = parse_blocks( $post_content ); foreach( $blocks as $block ) { if ( isset( $block['attrs']['align'] ) && 'left' === $block['attrs']['align'] ) { unset( $block['attrs']['align'] ); } } return implode( "\n\n", array_map( 'serialize_block', $blocks ) );
Essentially this copies existing logic which exists inside the editor into the backend.
Attachments (5)
Change History (20)
This ticket was mentioned in Slack in #core-editor by dmsnell. View the logs.
6 years ago
#2
@
6 years ago
Hi, I wonder if serialize_block()
from 47375.patch should make sure the array block keys exists, to avoid PHP notices if they don't exists in the input? It seems the $block['attrs']
and $block['innerContent']
is taken care of, but not $block['blockName']
.
Another thing that came to mind is the serialize
part of the function's name. When I read the ticket's title, I first thought of the PHP functions: serialize()/unserialize()
and the WordPress core functions: maybe_serialize()/is_serialized()
, but that's unrelated to this new function. I'm not sure if this is an issue at all, but I just wanted to mention my momentarily confusion with the name :-)
#3
@
6 years ago
Thanks for your review and feedback, @birgire!
make sure the array block keys exists
good call - I'll make sure this works and add the tests to verify it
the serialize part of the function's name
this comes with a symmetry to the JavaScript function of the same name. these are each one half of the parser/printer pair that establishes our ability to reliably store the data in HTML form.
I think I'd lean towards keeping it for the sake of consistency across the platforms (JS vs. PHP).
@
6 years ago
Handles non-block blocks (fallback blocks) and includes serialize_blocks()
function for handling the full list of blocks returned from parse_blocks()
#4
@
6 years ago
As I'll be away from this for a few months is there any chance someone could take over championing this ticket? I'd love to see it move into core. @jorgefilipecosta I remember seeing that you had similar work in another part of the plugin? Could you provide some guidance or help here?
#5
@
6 years ago
Hi @dmsnell, sure I will review this patch soon. The plugin has a similar function (not as complete) and it would be good to land this in core so we can remove it from the plugin.
cc: @noisysocks as he was the author of the serialize_blocks function available in the core, and may also have some insights.
#6
@
6 years ago
- Keywords has-patch has-unit-tests added
- Version set to trunk
I made some tests to the function, and it seems to work as expected. I replaced the Gutenberg function with the one implemented here, and the behavior was not changed. The code seems ready!
I applied some minor changes (type fixes, fix phpcs error, added comments required by phpcs, ...), the changes are available in the patch I just submitted.
I guess unless someone has some concern to raise this should be ready. @noisysocks, @pento any thoughts on this?
#7
@
6 years ago
good call - I'll make sure this works and add the tests to verify it
Thanks @dmsnell for the update
this comes with a symmetry to the JavaScript function of the same name. these are each one half of the > parser/printer pair that establishes our ability to reliably store the data in HTML form.
I think I'd lean towards keeping it for the sake of consistency across the platforms (JS vs. PHP).
... and for the link and clarification on the choice of the function's name. It makes sense.
What about adding a note to the new functions here, that they are not related to the serialize()
function in PHP?
I wondered about the 'innerHTML'
part, as it was not in 47375.patch, but I see it's been introduced in 47375-3.patch.
What about invalid $block
input of serialize_block( $block )
?
e.g. if $block
is not an array or $block['innerHTML']
array key is missing, then it looks like we would get a PHP notice there.
Same with the null === $block['blockName']
check in 47375-3.patch, seems to assume that the array key exists. It will give a PHP notice if it doesn't. Played with it here https://3v4l.org/5Vjh3
What do you think about making sure the input has the correct structure?
Thanks for the updated patch @jorgefilipecosta.
Here are few suggestions to 47375-4.diff:
- Missing end of line (dot) in few places (also in test file).
- I think it would be beneficial to the docs to add a more detailed description (i.e. the array keys
blockName
,attrs
,innerHTML
andinnerContent
) belonging to the block's input:@param array $block The block being rendered.
- Same for
* @param array $blocks The list of parsed block objects.
- What about adding
@covers ::serialize_block()
to the test class description, as all current tests are for theserialize_block()
function? - If added, then I think one could simplify the description of test methods, e.g.:
* Test: test_serializes_and_strips_away_core_namespace() should serialize and strip away core namespace.
to* The function should serialize and strip away core namespace.
#8
@
6 years ago
What about adding a note to the new functions here, that they are not related to the serialize() function in PHP?
Admittedly I have mixed feelings but I'm hesitant to add superfluous comments when I think they are incidentally needed. serialize()
is a wildly-generic term and I'm not personally familiar with any common trends that create some kind of specialized version of serialize()
for specific data types. I won't try and block an additional comment but in some ways I find the extra explanation a little unwarranted.
I wondered about the 'innerHTML' part
That's a shortcut based on the way the blocks are designed. We could have replaced it with (the arguably more reliable) $block['innerContents'][0]
but I left it short for clarity. Maybe that's the wrong call - not sure.
What about invalid $block input of serialize_block( $block )
What do you think about making sure the input has the correct structure?
I've intentionally not validated the inputs and while that may sound bizarre I do it for the sake of improving the quality. First of all, I think if we are worried about invalid input we might gain by addressing this from another angle - make it clearer that this should only get the contents of the block structure provided by parse_blocks()
or equivalents.
At this low level I consider it a game of tense alliances between all the different subsystems. If any block data is passed around which doesn't conform to our expectations than multiple systems will fail - to that end I'd rather we fail right out front at the boundary then cover the error - it's going to fail either here or right away somewhere else.
That block structure does bring certain assumptions and you're right to call them out. I'm not sure if I'll get this in but some links to documentation/specifications can hopefully help fill in the contextual gap here.
Otherwise awesome feedback @birgire and thanks so much @jorgefilipecosta for helping this along. You are both wonderful!
#9
@
6 years ago
@dmsnell thank you for an informative reply.
Admittedly I have mixed feelings but I'm hesitant to add superfluous comments when I think they are
incidentally needed. serialize() is a wildly-generic term and I'm not personally familiar with any
common trends that create some kind of specialized version of serialize() for specific data types. I
won't try and block an additional comment but in some ways I find the extra explanation a little unwarranted.
My initial thought was that this new PHP function was related to the commonly used serialize()
function in PHP, but I might be the only one experiencing that initial thought :-) Maybe a general dev-note introducing this new function, might then be more relevant instead?
That's a shortcut based on the way the blocks are designed. We could have replaced it with (the
arguably more reliable) $blockinnerContents?[0] but I left it short for clarity. Maybe that's the
wrong call - not sure.
That sounds good to me.
I've intentionally not validated the inputs and while that may sound bizarre I do it for the sake of
improving the quality. First of all, I think if we are worried about invalid input we might gain by
addressing this from another angle - make it clearer that this should only get the contents of the
block structure provided by parse_blocks() or equivalents.
At this low level I consider it a game of tense alliances between all the different subsystems. If
any block data is passed around which doesn't conform to our expectations than multiple systems will
fail - to that end I'd rather we fail right out front at the boundary then cover the error - it's
going to fail either here or right away somewhere else.
That block structure does bring certain assumptions and you're right to call them out. I'm not sure
if I'll get this in but some links to documentation/specifications can hopefully help fill in the
contextual gap here.
Yes I think it would be helpful to mention the relation to parse_blocks()
in the docs.
As this will become a public API (unlike the underscore _
prefixed functions), I can imagine it will be used in all kinds of situations and all kinds of inputs.
I think it could be helpful to validate the input, in such a way that the function will not output a "block zombie", for my lack of a better word :-)
This kind of output might look valid from a distance, but can scare the hell out of us when we look closer :-)
Here's an example:
serialize_block( [ 'blockName' => [] ] )
with output:
"<!-- wp:Array /-->"
I wonder if these kind of outputs should be allowed to go through?
This ticket was mentioned in Slack in #core-editor by gwwar. View the logs.
5 years ago
#11
@
5 years ago
Hi @birgire, thank you a lot for your review! Your suggestions were applied and a new patch submitted.
#12
@
5 years ago
@jorgefilipecosta thanks for adding in the comments and updating the patch. I'm back and can readdress this ticket now.
I'm nervous when I see us returning an empty string for invalid inputs. In the JavaScript side we already have the problem that if a block somehow fails during the parsing step it can mysteriously disappear. I'd rather return null
or throw an exception so that the caller can have better signaling that something failed, so that a developer can quickly see if they made a typo or mistake in sending in their block. Silently hiding failures is something I've seen come back and bite us too often.
Thoughts?
Also, what if we split out the type-checking into its own function. Seems like that might be usable in other places too.
If we like these ideas I can submit a new version of the patch soon.
Proposed function and associated tests