InnerBlocks: Fix regression where setting a template also locked it #7723
Conversation
InnerBlocks applied template sync on each update. This created a bug where if locking was not set when inserting or removing a block the template mechanism reverted the action. This commit makes sure we only sync with the template if templateLock equals "all" or if the template contains inner blocks and the blocks do not contain them any inner block.
jorgefilipecosta
added
the
[Feature] Nested / Inner Blocks
jorgefilipecosta
force-pushed
the
fix/innerblocks-template-sets-a-locking
branch
from
8aa2481
to
128946a
Compare
There was a problem hiding this comment.
What cases is this lifecycle method trying to cover? We should be clear with this via inline comments and/or tests. I fault myself for this with recent refactorings. One case I don't think we're covering is the template change; if the developer passes a new template, don't we want to effect it even if there is no "mismatch" ? Seems like the fix may be to change isTemplateInnerBlockMismatch to account for the templateLock === 'all', yeah?
|
|
||
| this.updateNestedSettings(); | ||
| if ( templateLock !== 'all' && block.innerBlocks && block.innerBlocks.length > 0 ) { |
There was a problem hiding this comment.
We're checking truthiness of block.innerBlocks before testing length here but not below in the assignment of isTemplateInnerBlockMismatch. We should be consistent one way or the other, depending on whether we actually need to be defensive.
|
Also, there's too much logic going on in a lifecycle method here. This should probably be separated out and clearly documented (incl. self-documenting named function). Can this be consolidated into |
We should only do that for the case where templateLock is all or for the case where we don't have any innerBlocks yet. For the other cases, as we can remove blocks (or even insert if locking != insert), on template change, we should not apply the sync because we would not know if we are removing something the user did intentionally. |
|
Why can we make that assumption in component updates, but not for component mount. Wouldn't it also be an issue for reloading an editor where the template doesn't match the blocks from in content? |
|
I don't recall why we care to look at the template blocks length at all to determine mismatch; I see it's buggy without, but bugs I'm observing seems not totally related. Digging more... |
|
Closed in favor of #7732. |
Regressed in: #7234
InnerBlocks applied template sync on each update. This created a bug where if locking was not set when inserting or removing a block the template mechanism reverted the action.
This commit makes sure we only sync with the template if templateLock equals "all" or if the template contains inner blocks and the block does not contain any inner block.
How has this been tested?
I checked columns block continues to work as before.
I added the test blocks available in https://gist.github.com/jorgefilipecosta/2bac096fe3be7f6ff0de7452292161cb.
I added the locking all and checked we cannot insert blocks on the locking all. But we can insert on the no locking block inside (on master we can't).
I added the "no locking" and "locking unset" blocks and verified in both of them I can insert an image.