Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform template synchronisation in the InnerBlocks component #11681

Open
john-freedman opened this issue Nov 9, 2018 · 23 comments

Comments

@john-freedman
Copy link

@john-freedman john-freedman commented Nov 9, 2018

Describe the bug
When creating a post that uses 'template_lock'=>'all' and contains InnerBlocks with templateLock={false}, everything works as it should (top level of the template is locked, but InnerBlock sections are unlocked). But after adding content to those InnerBlocks using the editor, next time you edit that post, Gutenberg prompts, "The content of your post doesn’t match the template assigned to your post type."

To Reproduce
Steps to reproduce the behavior:

  1. Make a custom post type with a template defined:
register_post_type( 'test', array(
  'label' => 'Test',
  'public' => true,
  'show_in_rest' => true,
  'template' => array(
    array( 'test/test_block', array(), array() ),
  ),
  'template_lock' => 'all'
) );
  1. registerBlockType
registerBlockType( 'test/test_block', {
  // usual code here

  edit( { className } ) {
    return (
      <div className={ className }>
        <InnerBlocks templateLock={ false } />
      </div>
    );
  },
  save() {
    return (
      <div>
        <InnerBlocks.Content />
      </div>
    );
  }
}
  1. In the WordPress admin, create a new post of this post type, put anything inside the block that's on this page, and publish the post.
  2. Refresh the post edit page (or leave this page and navigate to it again) and you'll see this error:

Expected behavior
I think there isn't supposed to be an error, but I'm not sure. I can't find this specific issue mentioned anywhere, so I could easily just be doing something wrong, but I can't figure out what it is. If it is user error, then I'm sorry for the trouble.

Additional context

  • Gutenberg 4.2.0
  • Wordpress 4.9.8
@skript-cc

This comment has been minimized.

Copy link

@skript-cc skript-cc commented Nov 15, 2018

I have exactly the same problem when setting a custom template for the 'post' post type. My template has the same template locking setup.

@janeschindler

This comment has been minimized.

Copy link

@janeschindler janeschindler commented Nov 18, 2018

I have the same problem with the same setup.

@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Feb 1, 2019

Yes, I think nested templates validation should not happen at the root level like it's done now but it should happen inside the InnerBlocks component as it's the one that is aware of its locking config.

@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Feb 1, 2019

@youknowriad youknowriad changed the title Error after publishing template locked post Perform template synchronisation in the InnerBlocks component Feb 1, 2019
@FerrielMelarpis

This comment has been minimized.

Copy link

@FerrielMelarpis FerrielMelarpis commented Apr 22, 2019

Is there a workaround for this issue?

@mmtr

This comment has been minimized.

Copy link
Contributor

@mmtr mmtr commented Jul 3, 2019

Is there a workaround for this issue?

You can manually force the template validity by dispatching a setTemplateValidity action:

dispatch( 'core/block-editor' ).setTemplateValidity( true );
@chrisvanpatten

This comment has been minimized.

Copy link
Member

@chrisvanpatten chrisvanpatten commented Jul 9, 2019

This bit me today as well. I solved it with the trick @mmtr shared; here's my full code if anyone wants to steal it:

/**
 * WordPress dependencies
 */
import { compose } from '@wordpress/compose';
import { withDispatch } from '@wordpress/data';
import { InnerBlocks } from '@wordpress/editor';

/**
 * Container block
 *
 * @return {Object}
 */
const Editor = () => (
	<InnerBlocks templateLock={ false } />
);

// This is a hack which forces the template to appear valid.
// See https://github.com/WordPress/gutenberg/issues/11681
const enforceTemplateValidity = withDispatch( ( dispatch, props ) => {
	dispatch( 'core/block-editor' ).setTemplateValidity( true );
} );

export default compose(
	enforceTemplateValidity,
)( Editor );

Definitely feels hacky though; I'd love to see it resolved in a better way :)

noahtallen added a commit to Automattic/wp-calypso that referenced this issue Aug 1, 2019
This is VERY much a hack around
WordPress/gutenberg#11681

Previously, it was fine to do this just once at the beginning. But we
discovered that it also happens when performing undo.
Granted, performing undo might do something else, but my best guess is
that it's related to the above issue.
@danielpost

This comment has been minimized.

Copy link

@danielpost danielpost commented Aug 5, 2019

@chrisvanpatten Does your fix also work when using the Columns block in the InnerBlocks? That's when I still get the error—other blocks seem to work fine.

EDIT: For context, this is how I'm registering the block. The underlying motivation is having a CPT template where some blocks are fixed and unchangeable, followed by this empty block where users can add anything they want (since the Group block isn't in core yet).

const { compose } = wp.compose;
const { __ } = wp.i18n;
const { withDispatch } = wp.data;
const { registerBlockType } = wp.blocks;
const { InnerBlocks } = wp.editor;

registerBlockType('vo/empty-block', {
    title: __('Empty Block'),
    icon: {
        src: 'media-spreadsheet'
    },
    category: 'common',
    supports: {
        inserter: false,
        reusable: false,
        html: false
    },
    // This is a hack which forces the template to appear valid.
    // See https://github.com/WordPress/gutenberg/issues/11681
    edit: withDispatch(dispatch => {
        dispatch('core/block-editor').setTemplateValidity(true);
    })(() => <InnerBlocks templateLock={false} renderAppender={() => <InnerBlocks.ButtonBlockAppender />} />),
    save: props => {
        return <InnerBlocks.Content />;
    }
});
getdave added a commit to Automattic/wp-calypso that referenced this issue Aug 13, 2019
This is VERY much a hack around
WordPress/gutenberg#11681

Previously, it was fine to do this just once at the beginning. But we
discovered that it also happens when performing undo.
Granted, performing undo might do something else, but my best guess is
that it's related to the above issue.
@jrchamp

This comment has been minimized.

Copy link
Contributor

@jrchamp jrchamp commented Sep 26, 2019

According to GitHub, @mmtr @noahtallen and @getdave (all from Automattic) have touched commits that work around this issue by suppressing the warning. @chrisvanpatten is a Gutenberg core team member and provided a similar workaround posted above.

Please tell me someone is officially working to fix the root cause. Otherwise, we might as well remove the warning entirely. 😞

Edit: I'm sorry to call people out, but it seems like there's smart, capable people here on this issue that has been languishing for almost a year. I believe in you and am rooting for you.

@noahtallen

This comment has been minimized.

Copy link
Contributor

@noahtallen noahtallen commented Sep 26, 2019

Thanks for calling us out! I mean it! Speaking for my own project, we were trying to ship a fix quickly which is why we didn't focus on this side of things. I think I'll have some extra cycles in the next week or two and I'll try to poke around at this.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

@chrisvanpatten chrisvanpatten commented Sep 28, 2019

@jrchamp I agree completely that this is an important issue which needs to be solved. I think it's also a relatively complicated one; the "fix" above is really a hack, and not something that should be integrated into Gutenberg itself.

Prep for WordPress 5.3 has definitely taken priority for much of the core team, but I'll try to bring this ticket up in an upcoming Gutenberg meeting, and I'd encourage you to join as well (9am Eastern time in #core-editor in WordPress Slack).

@davidchoy

This comment has been minimized.

Copy link

@davidchoy davidchoy commented Oct 1, 2019

In an episode of terrible lazyness, until this is fixed or I learn exactly how withDispatch works, I'm using the following, less-subtle hack (SCSS->CSS enqueued by admin_enqueue_scripts) – hey, it will probably encourage me to think of a better solution every-time I see the modified warning.

div.components-notice-list {
  div.editor-template-validation-notice.components-notice.is-warning {
    >div.components-notice__content {
      &:before {
        //content:'Notice: This is a modified template. (The content of your post doesn’t match the template assigned to your post type.)';
        content:'Note: This is a modified template. This message may be removed in a future update.';
        display:block;
        margin-bottom: 1rem;
        margin-left:0.5rem;
      }
      >p {
        display:none;
      }
      >div {
        //uncomment for the even worse solution of hiding all errors that fall in this this class of div
        //display:none;
      }
    }
  }
}
@noahtallen

This comment has been minimized.

Copy link
Contributor

@noahtallen noahtallen commented Oct 10, 2019

cc @youknowriad I started diving into this issue and was curious if you could give a bit more guidance. Here's what I found poking around the current code:

  • An overall template can currently be set at a high level: state.settings.template in the block editor provider.
  • Template locking can be also be defined at that high level: state.settings.templateLock.
  • doBlocksMatchTemplate validates all nested blocks from the top down in editor provider as a whole, never taking nested templateLocks into consideration (as far as I can tell in the code :))
  • Overall template validity is only run on RESET_BLOCKS action, which happens when the block editor provider mounts or the external property for blocks changes (i.e. it doesn't continuously happen as you make changes to blocks)
  • InnerBlocks does its own kind of template synchronization if a template is given as a prop (templates aren't required), but never checks template validity in such a way which would create the notice we see. This mechanism is totally separate from the global template.

As mentioned, this architecture results in this bug, since it doesn't allow for different locks to apply to different levels. Global template validation in the editor provider currently has no concept that individual blocks could have their own template and rules for the template.

I think the path forward for the template synchronization is to remove the "nesting" entirely and preform the synchronization at the "InnerBlocks" component level like suggested in this issue #11681

I'm trying to figure out what this would look like.

In InnerBlocks, we already do perform synchronizeBlocksWithTemplate when the template changes. This happens automatically when there is a difference in what the blocks should look like, so it does not show a notice. Interestingly, it does not use validateBlocksToTemplate, it just uses isEqual( nextBlocks, innerBlocks ) and isEqual( template, prevProps.template ).

So InnerBlocks is already validating the template it gets through props separately from the global template. So a way to fix this bug would be to not continue recursing in validateBlocksToTemplate if we reach a block which does its own validation.

However, the current implementation would not provide a way to do this - in that function, the block data does not contain information about its own template, using InnerBlocks, or having a template lock.

I would greatly appreciate some guidance if folks have some ideas of what they'd like to see happen :)

@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Oct 10, 2019

So a way to fix this bug would be to not continue recursing in validateBlocksToTemplate if we reach a block which does its own validation.

However, the current implementation would not provide a way to do this - in that function, the block data does not contain information about its own template, using InnerBlocks, or having a template lock.

Yeah, this seems like the crux of the problem. Potentially, we could a block API to define the behavior but it doesn't seem perfect as the behavior could change from one instance to another of the same block type.

I wonder if we could do something like:

  • Only perform validation in BlockList but without validating the nested blocks (which is rendered in InnerBlocks and also at the root level) and have a way for sub-blocks to access their "part" of the "global template" to perform the validation.

I'll also ping @jorgefilipecosta he's the expert here :)

@noahtallen

This comment has been minimized.

Copy link
Contributor

@noahtallen noahtallen commented Oct 10, 2019

have a way for sub-blocks to access their "part" of the "global template"

I wonder how this should integrate with the InnerBlocks template. So I can say <InnerBlocks template={} />, but what if I make that template different from the global template?

@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Oct 10, 2019

@noahtallen Good question, I think the prop should have priority in these cases if the template locking is explicitely set as a prop to innerBlocks.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Oct 11, 2019

Hi all, thank you for the explorations on this issue 👍 @noahtallen, @youknowriad To me, it seems the solution to this problem is not to continue recursing in validateBlocksToTemplate at all. Let's only validate if the top-level blocks match. Then if descendent blocks have a locking, their validation mechanics will trigger. If they don't have locking, we can see that specific inner block area as a not locked and a controlled part where the user is free.

@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Oct 11, 2019

yeah, that's exactly what I was suggesting as well.

@noahtallen

This comment has been minimized.

Copy link
Contributor

@noahtallen noahtallen commented Oct 11, 2019

This approach would be very straight forward :) One thing I'm curious about, though. In the docs we specify that nested blocks in the templates are viable to support container blocks. do we not support that case any more, or is there somewhere else where we need to add validation to replace what was happening with the recursion?

Thanks for the tips, btw, it's very helpful to understand the context :)

@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Oct 14, 2019

For nesting, I think if there's no strong lock or if no template is provided as a prop, the global template content can be used.

This is me saying, it should still work.

@jrchamp

This comment has been minimized.

Copy link
Contributor

@jrchamp jrchamp commented Oct 16, 2019

My use case (so you have another example to text against):

  • Custom post type
  • Locked template that specifies the top-level blocks (so we can enforce control of the valid blocks and their order)
  • Some of those top-level blocks have InnerBlocks which must unlock the template so they can allow for repeating child blocks (so users can add multiple allowed child blocks to repeat-enabled parents)

Example:

  • Class Subject post type with {templateLock: true, template: [['activity-section'], ['resource-section']]}
  • Activity Section block, with {templateLock: false}, allowing children "Activity Type" blocks
  • Activity Type block, inheriting {templateLock: false} from parent, with children: "Activity" blocks, specifying the Activity type label
  • Activity block, without children, structuring the Activity data
  • Resource Section block, with {templateLock: false}, allowing children "Resource Type" blocks
  • Resource Type block, inheriting {templateLock: false} from parent, with children: "Resource" blocks, specifying the Resource type label
  • Resource block, without children, structuring the Resource data

For my use case, I would be open to specifying my post-level template all the way to the children, such as by specifying that a block is "repeatable" and "optional".

However, if someone's use case is less strict than mine, they may not know all of the blocks that might be in their unlocked children. In that situation, it's probably better to only validate one level at a time and delegate the validation to the child block. Of course, that might be tricky if they have multiple InnerBlock components in a single parent block.

@noahtallen

This comment has been minimized.

Copy link
Contributor

@noahtallen noahtallen commented Nov 5, 2019

It's really funny, I've been working on several tasks recently and everything seems to point back to this issue. I haven't yet created a PR because I'm still trying to piece together how everything works. I think this has to be a top priority so that we can make some progress. Flexible and powerful templates are super important for advanced block needs.

Since this has taken me a while to get my head around, I want to outline what I've learned for anyone who is following along, and gain extra clarity through writing.

To start, knowing how blocks are rendered is important. Here's the component structure (ish) in the editor. The blocks are actually rendered in BlockListBlock (or some sub-component), but everything above it is just managed mostly with arrays of client IDs to determine ordering.

- ** BlockEditorProvider **
    - BlockList (rendered by edit-post/layout/visual-editor)
        - BlockListBlock (rendering some block like 'core/some-block-here')
            - ** InnerBlocks **
                - BlockList (etc)

Blocks with ** asterisks ** around them perform template validation of some sort

The BlockEditorProvider makes sure that the blocks in the block-editor store match the template in the overall settings of the provider. InnerBlocks makes sure that its children blocks match the template parameters given as props. The unfortunate aspect of this is that block validation mechanisms are different between the two.

Why is this problematic? Introducing new APIs and settings for block templates is going to be difficult. For example, what if I want to add support in Gutenberg for this layout:

Allow n blocks of any type, but there must be at least one block of type x among them.

Not only do I have to make sure that InnerBlocks supports this, but I also have to make sure that this is supported at the global, BlockEditorProvider level so that the top layer of blocks can be validated.

As noted by this issue, there are several other possibilities for block templates. In fact, many things can be solved by block templates. What if I want to make sure that a specific block is non-removable? I could create a template setting which makes sure that the block isn't removed. Different block actions could pull from the same template API instead of doing a rough check like isMovable: 'all' !== templateLock. But to get any of this working well, we need one source of truth for block templates.

After understanding the above, I finally get why this could make sense:

Perform validation in BlockList but without validating the nested blocks (which is rendered in InnerBlocks and also at the root level) and have a way for sub-blocks to access their "part" of the "global template" to perform the validation. (@youknowriad)

In other words, we stop validating blocks both at the BlockProvider level and the InnerBlocks level and instead combine that logic into one, consistent API which wraps BlockList. getTemplateLock( rootClientId ) contains part of the answer (it can retrieve the template settings for arbitrary clientIds), but only for nested blocks with a set template, not for a global template. If we want to validate blocks to a global template, we'd need to expand that so that it can also check the client ID against the global template:

I think if there's no strong lock or if no template is provided as a prop, the global template content can be used. (@youknowriad)

The challenge here will be parsing the template tree to see what matches the given clientId. It's hard because the template is just defined with block names.

Here are some action items that make sense to me. I'm open to lots of feedback on this.

    • Perform template validation in the BlockList component.
        • Add the global template settings under the blockListSettings state
        • Local templates should take precedence over the global template.
        • Create logic which also parses the global template given an arbitrary client ID.
    • Remove block validation from the BlockEditorProvider level in favor of 1.
        • Make sure that the notice on invalid template still displays and works properly
    • Remove block validation from InnerBlocks in favor of 1.
    • (future) Create a new way to consume this API in order to power things like isMovable, isLocked, etc. etc. It should be more specific and consistent so that it's easy to modify the block actions available based on the template.
@youknowriad

This comment has been minimized.

Copy link
Contributor

@youknowriad youknowriad commented Nov 11, 2019

@noahtallen Nice summary and I think you got everything right.

Create logic which also parses the global template given an arbitrary client ID.

This is indeed the most difficult problem, we already have a helper that does this "somehow" (the sync mechanism), we can use some of it or make it more flexible. Also maybe instead of having a selector/function to retrive the global template per clientId, maybe a simpler way is to instead provide that template as we render the tree using a prop/context: basically while looping through the blocks in BlockList, also loop through the global template and make the sub-template available to the sub block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.