Blocks: Deprecate isValidBlockContent
#38794
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +98 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
gziolo
added
[Feature] Block API
dmsnell
force-pushed
the
blocks/deprecate-is-valid-block-content
branch
2 times, most recently
from
2787918
to
a51947f
Compare
dmsnell
force-pushed
the
blocks/deprecate-is-valid-block-content
branch
from
a51947f
to
36336cf
Compare
10 tasks
dmsnell
added a commit
that referenced
this pull request
Feb 22, 2022
Potentially this is a conflict with #38794 and once `isValidBlockContent` is gone we might be able to remove this change.
dmsnell
force-pushed
the
blocks/deprecate-is-valid-block-content
branch
from
36336cf
to
d4998ae
Compare
dmsnell
added a commit
that referenced
this pull request
Feb 23, 2022
Potentially this is a conflict with #38794 and once `isValidBlockContent` is gone we might be able to remove this change.
scruffian
approved these changes
Mar 2, 2022
dmsnell
added a commit
that referenced
this pull request
Mar 4, 2022
Potentially this is a conflict with #38794 and once `isValidBlockContent` is gone we might be able to remove this change.
|
I didn't find much on sourcegraph: There were a few matches on the wpdirectory.net, though:
Most of them seems like a bundled version of Gutenberg, but I didn't dive super deep into every single one. Overall, it seems like it isn't a wildly popular function.
👍
It is part of the public API so I'd rather not remove it, even if there aren't many publicly facing results. The way I saw deprecations work in Gutenberg was marking the function as deprecated, which you already did, and then wait at least two Gutenberg versions or longer if needed. A sensible way to proceed here would be to merge this PR, make the change visible on slack, and then create an issue to remove the function later on. Even if it ends up being removed after WP 6.0 is released to give the plugin authors time to adjust, that sounds fine. |
adamziel
approved these changes
Mar 8, 2022
|
also cc @ellatrix @mcsf @noisysocks |
dmsnell
force-pushed
the
blocks/deprecate-is-valid-block-content
branch
from
d4998ae
to
e990701
Compare
Thanks for looking that up @adamziel! that's a great idea I hadn't considered. looks good then. I didn't suspect we'd find much; still could be some uses out there but I bet the risk is low here. I added CHANGELOG entries but I don't know what I'm doing, so I added them under |
dmsnell
added a commit
that referenced
this pull request
Mar 9, 2022
Potentially this is a conflict with #38794 and once `isValidBlockContent` is gone we might be able to remove this change.
dmsnell
added a commit
that referenced
this pull request
Mar 11, 2022
Potentially this is a conflict with #38794 and once `isValidBlockContent` is gone we might be able to remove this change.
dmsnell
added a commit
that referenced
this pull request
Mar 14, 2022
Potentially this is a conflict with #38794 and once `isValidBlockContent` is gone we might be able to remove this change.
The `isValidBlockContent` function attempts to validate a block by re-generating its output but does so by ignoring any `innerBlocks` that might exist. Since blocks can change their output based on the presence or content of its inner blocks this will lead to incorrect results from the validator. `validateBlock` also makes this mistake however it is fixable since it requires passing in the block itself (fixable in another patch once there are no longer two interfaces for what appears to be the same purpose). `isValidBlockContent` doesn't pass in the block and so is fundamentally limited in being able to validate properly. This limitation also means that we can't rely on `isValidBlockContent` once we start threading source information into blocks; that is, as we improve our ability to catch and fix or preserve issues with the block markup, this function provides no way forward to incorporate those updates. After scouring through the commit logs I was unable to find a clear purpose for this second validation method and in Core it only seems to be used as a convenience function for tests. We're deprecating it in this patch because it's an exposed public interface and it's not clear if anyone relies on it.
dmsnell
force-pushed
the
blocks/deprecate-is-valid-block-content
branch
from
2b7bb5b
to
7b13d9e
Compare
bph
added
the
Needs Dev Note
37 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The
isValidBlockContentfunction attempts to validate a block byre-generating its output but does so by ignoring any
innerBlocksthat might exist. Since blocks can change their output based on the
presence or content of its inner blocks this will lead to incorrect
results from the validator.
validateBlockalso makes this mistake however it is fixable sinceit requires passing in the block itself (fixable in another patch
once there are no longer two interfaces for what appears to be the
same purpose).
isValidBlockContentdoesn't pass in the block andso is fundamentally limited in being able to validate properly.
This limitation also means that we can't rely on
isValidBlockContentonce we start threading source information into blocks; that is, as
we improve our ability to catch and fix or preserve issues with the
block markup, this function provides no way forward to incorporate
those updates.
After scouring through the commit logs I was unable to find a clear
purpose for this second validation method and in Core it only seems
to be used as a convenience function for tests. We're deprecating it
in this patch because it's an exposed public interface and it's not
clear if anyone relies on it.
Testing Instructions
There are two testable changes in this PR:
isValidBlockContent?What do we think about the deprecation? Should we schedule removal?
Should we outright remove it instead?
validateBlockwe haveremoved the tests and constraints on
isValidBlockContent. We couldpreserve them but I want to avoid as much confusion as possible and
suggest people only use
validateBlock.The only affected code in Core should be the tests, so if they pass
then there's nothing more to test.
Types of changes
As part of ongoing work to preserve existing unrecognized content (via invalid or missing blocks) this patch is deprecating an interface for block validation that overlooks inner blocks and parse failures. Because of this and because of the broader issue the editor has a tendency to corrupt or lose content on save, even for unrelated changes.
This change works to make it easier to account for inner blocks on validation as well as better preservation of those unrecognized bits.
Checklist:
*.native.jsfiles for terms that need renaming or removal).