WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10k stars 4.02k forks source link

Dynamic blocks: default values for attributes aren't converted into html comments #7342

Open katerlouis opened 6 years ago

katerlouis commented 6 years ago

Describe the bug When an attribute isn't changed, the value will not be written into the serialized html comment, which in fact, makes it unaccessible for PHPs render_callback-function.

To Reproduce Steps to reproduce the behavior:

  1. Make a custom block
  2. Add a sourceless attribute with default value
    attributes: {
    myNumber: {
        type: "number",
        default: 1, // this number won't appear in the html comments
    }
    }
  3. Do not alter the attribute
  4. Save the content
  5. var_dump() the $attribues in 'render_callback 5b. or look into database, the html comment doesn't contain 'number'
  6. Be sad 'number' isn't there :'(

Expected behavior To avoid syncing the defaults in both php and js, I expect gutenberg to save defaults into the html comment aswell.

Desktop (please complete the following information):

Additional context

katerlouis commented 6 years ago

Slight push on this one. The more attributes I have, the more this is bothering me–

Is this intended?

danielbachhuber commented 6 years ago

Hi @katerlouis,

What you've documented is the current behavior, yes. I've flagged this for review; we may consider changing the behavior in the future.

Related #4494 #5739 #2751

katerlouis commented 6 years ago

@danielbachhuber Is there a trick / workaround which forces gutenberg to write the value in the comment?

danielbachhuber commented 6 years ago

@katerlouis Not to my knowledge, no.

gziolo commented 6 years ago

@matias or @aduth, do you happen to know how this can be solved with the current state of Gutenberg?

aduth commented 6 years ago

With moving block definitions to the server, I expect the preparation of generating attributes for render_callback should be accounting for the default values of attributes defined in register_block_type.

That default values are not encoded in comments is the expected behavior.

https://github.com/WordPress/gutenberg/blob/7a878e003537cc4ca6c13833cc942fd806036e6c/blocks/api/serializer.js#L154-L157

See also #1905

aduth commented 6 years ago

I'm inclined to close this under the umbrella of #2751, as it essentially becomes a non-issue once implemented.

For near-term, as a plugin developer you're best off to register your block attributes on the server. They should be inherited into the client automatically. If there's an issue with this, it can be addressed separately.

Otherwise, it's expected that the server is respecting default values assigned to attributes in prepare_attributes_for_render.

https://github.com/WordPress/gutenberg/blob/7a878e003537cc4ca6c13833cc942fd806036e6c/lib/class-wp-block-type.php#L144-L146

elliotcondon commented 5 years ago

Although closed, I would like to add to this topic.

An attribute's default value should not be ignored when generating the block HTML comment. Think of the "align" attribute which is used to set the width of a block (full, wide, default, etc).

The Block API Handbook shows that this attribute can be given a default value, which is a good feature. For example, a custom "gallery" block type can default to "full".

I'm not sure if this issue affects a standard block type (because the align attribute will be rendered as actual HTML), but the issue becomes apparent when working with a dynamic block type.

A dynamic block relies on all the attributes to be available to its render_callback function as there is no HTML saved by the block. The "full" align attribute mentioned earlier is never sent to the callback which makes it impossible to know what the alignment should be.

Upon further reading, it seems that the "solution" is to define the default value value within the PHP register_block_type() function. This would provide a way for the render_callback to find the same default value.

The issue with this "solution" is that all dynamic blocks are vulnerable to having their "align" setting bulk updated if the developer decides to change this default value setting later on. The blocks don't have any "align" attributes saved within the HTML comments, and will default to the "new default value".

This could cause a lot of headaches to web developers.

danielbachhuber commented 5 years ago

@aduth @mtias Given #2751 won't land for 5.0, does it make sense to implement this behavior? ^

aduth commented 5 years ago

The issue with this "solution" is that all dynamic blocks are vulnerable to having their "align" setting bulk updated if the developer decides to change this default value setting later on. The blocks don't have any "align" attributes saved within the HTML comments, and will default to the "new default value".

If there was no explicit user interaction, I'd see this as working as intended in treating it as an undefined value which should defer to the block's default, even if that default changes over time.

Could this be confusing for a user in that their content could suddenly appear different? Perhaps, but the use-case for a block changing its default seem uncommon such that in the event it was to occur, I think it'd be more confusing to the user that the new default not take effect for a block in which they had made no choice to its value.

Now, if the user had taken an explicit action in choosing a value which happened to align with the block's current default at the time, I'd probably agree that this should be included in the comment demarcation, such that the choice be respected should the default be updated in the future.

None of this speaks to the availability of the value on the server, which I still hold is better accounted for by #2751.

elliotcondon commented 5 years ago

Hi @aduth Thanks for the reply and input.

I still believe this is something that should be improved but is ultimately your decision.

Here is a workaround I have implemented to allow a "default value" to be defined whilst allowing the block to include this attribute value to be serialized:

/**
*  withDataAlign
*
*  Fixes a bug where block attribute "default values" are not saved in serialized data.
*  Extend the default BlockListBlock component and apply a default "align" value allowing
*  Gutenberg to serialize the "align" setting (normally ignored).
*
*  @see     https://github.com/WordPress/gutenberg/issues/7342#issuecomment-435371583
*  @date    5/11/18
*  @since   5.8.0
*
*  @param   object BlockListBlock The BlockListBlock element.
*  @return  createElement
*/
var withDataAlign = wp.compose.createHigherOrderComponent(function( BlockListBlock ) {
    return function( props ) {

        // get blockType
        var blockType = wp.blocks.getBlockType( props.block.name );

        // add default
        if( props.block.attributes.align === undefined && blockType.align ) {
            props.block.attributes.align = blockType.align;
        }

        // return
        return wp.element.createElement(
            BlockListBlock,
            props
        );
    };
}, 'withDataAlign' );
wp.hooks.addFilter( 'editor.BlockListBlock', 'acf/with-data-align', withDataAlign );

Hopefully this can help other developers experiencing the same problem or spark some more conversation.

aduth commented 5 years ago

An issue you may encounter in this implementation is that you're mutating the attributes object directly, rather than going through the store's actions (e.g. wp.data.dispatch( 'core/editor' ).updateBlockAttributes). This could result in some subtle unexpected behaviors as the editor won't necessarily be immediately aware of the change in attribute value and the impact this should have on various behaviors (e.g. is saveable, is dirty, undo history, etc).

elliotcondon commented 5 years ago

@aduth Thanks for the tip. Is this similar to the block.setAttributes() function? I've tried this, but found that it caused a UI glitch where the UI is initially rendered without the "default value" and then another render is triggered ~100ms later with the new value.

aduth commented 5 years ago

Is this similar to the block.setAttributes() function?

Under the hood, they are effectively the same (with some small differences).

caused a UI glitch where the UI is initially rendered without the "default value" and then another render is triggered ~100ms later with the new value.

This is the sort of subtle issue I had in mind in my previous issue.

I wonder if this may be the same issue which is set to be addressed in #11777, specifically affecting components which dispatch during their mount.

korynorthrop commented 4 years ago

I'm curious if @elliotcondon's approach above https://github.com/WordPress/gutenberg/issues/7342#issuecomment-435724307 is still the best method to getting dynamic blocks' default values serialized into the HTML comments for the Post. I find myself in a situation where I need these default values available in the HTML comments.

spencerbeggs commented 4 years ago

A little hacky, but you can just conditionally set the default value:

const Editor = props => {
    const { setAttributes, attributes } = props;
    let { myAttribute } = attributes;
    if (!myAttribute) {
       setAttributes({ myAttribute: "foobar" });
   }
   return <div>{myAttribute}</div>;
}

registerBlockType("my/hack", {
  title: "Hack",
  icon: "list",
  category: "common",
  attributes: {
    myAttribute: {
      type: "string"
    }
  },
  edit: props => <Editor {...props} />,
  save: ({ attributes }) => <MyComponent {...attributes} />
});
elliotcondon commented 4 years ago

@spencerbeggs - This works, but causes a "double render" refresh.

spencerbeggs commented 4 years ago

@elliotcondon Only the first time you open the editor though. Like I said, it's a hack. My other solution was to double the needed attribute, keep them in sync and then destucture it out before saving. But I figure a slightly blinky UI was better.

emilio-martinez commented 4 years ago

Just ran into this issue.

To add more confusion, I've noticed that for some attribute types (notably booleans) the attribute isn't even passed into the render_callback when it's the same as the default—regardless of whether it has changed in the editor or not! 🙄 So the attribute is literally unset in the PHP array which is particularly annoying because it forces me to handle fallback behaviors for every attribute with something like

$my_attr = isset($attributes['myAttr']) ? $attributes['myAttr'] : $my_attr_default;

This lead me to try defining the attributes in PHP via register_block_type which as alluded to elsewhere in this thread, does reliably add the attributes into the render_callback. Not only that, but when setting the attributes via PHP those become the attributes, and seem to override anything that's later set in Javascript via registerBlockType.

Unclear why these two seemingly equivalent APIs behave differently, but at this point I'm inclined to move as much of the block definition as I can to a shared format (json?) and pull data into either environment as needed.

Please just somebody review the APIs and normalize them 🤦 this is hardly the first time I've bumped into issues where both of these APIs behave differently.

mcsf commented 4 years ago

Please just somebody review the APIs and normalize them 🤦

This isn't a helpful way of engaging. A lot of people are working to solve this problem in a way that meets very different and almost conflicting requirements. That said:

Unclear why these two seemingly equivalent APIs behave differently, but at this point I'm inclined to move as much of the block definition as I can to a shared format (json?) and pull data into either environment as needed.

That has been the direction for a long time, in incremental steps. Most recently: #22491. With that in, it should now be much more feasible for the back-end block renderer to consider default attribute values.

gziolo commented 4 years ago

Extending on what @mcsf shared, as of today in Gutenberg plugin, all blocks but Embed set of blocks are registered on the server with all defaults for attributes listed in their definitions (when provided). I hope we can backport everything on time for WP 5.5, but I'm positive that it'll be the case

emilio-martinez commented 4 years ago

First and foremost, excuse the frustration expressed before—you're correct, not constructive.

All the guidance (or most of it anyway) I've seen across the web seems to suggest that registering attributes first and foremost happens via the JavaScript API, not the PHP API. This is including the WordPress Block Tutorial and the examples codebase, which presumably would be the starting point for most developers venturing into creating custom blocks. Of course this is without mentioning countless blog posts and tutorials out there which WordPress doesn't have control over.

If this has been the direction for a while, perhaps it hasn't been portrayed clearly enough? What's a good place to start steering people in the direction of the PHP API for attribute registration? Submit PRs to the Handbook docs?

emilio-martinez commented 4 years ago

Question: Is the intent roughly to move in the direction of defining blocks via register_block_type_from_metadata as shown below by passing a json file path or dir?

https://github.com/WordPress/gutenberg/blob/b83f94845501378dfd78f1a10db785e05dee2d87/lib/blocks.php#L96-L115

mcsf commented 4 years ago

If this has been the direction for a while, perhaps it hasn't been portrayed clearly enough? What's a good place to start steering people in the direction of the PHP API for attribute registration? Submit PRs to the Handbook docs?

I would guess that information spreads unevenly depending on the source and the message, and so you'll find seemingly conflicting information despite efforts to communicate an intended direction.

The first discussion, #2751, is almost three years old. Aiming to solve the question of server-side block awareness, it argued in favour of environment-agnostic ways to describe a block type, including its attribute schema. The most natural candidate was JSON, but at the time it wasn't yet clear whether the source should be a declaration in PHP which could generate JSON, or vice-versa.

More recently, a long and more visible discussion on this was #13693. It referenced #2751 for context, saw a ton of discussion, and produced a Markdown document (now outdated) explaining where we were headed with JSON declarations. This was early last year. A still open follow-up issue is #16209.

Meanwhile, block registration has been increasingly happening on the server in Core/Gutenberg. Recently, #22491 was the culmination of this. The idea is that we can all benefit from the server having knowledge of all block types, as opposed to just dynamic ones (an earlier approach). Also, since early on, the very useful (and official) block creation tool @wordpress/create-block automatically creates a PHP-based block type declaration. So, a person using core blocks and creating new blocks with the official tool is automatically benefiting from server-side block registration.

That said, client-side registration has never been compromised or discouraged (and in fact server-side registration ultimately triggers client-side registration) and was the first-ever API for block registration. That, combined with its overall simplicity, make it a great showcase for tutorials and the like (e.g. a single JS file can register a block and define its edit and save methods). Plus, as you point out, documentation can become out of sync with emerging priorities. It's not that the docs are now wrong, but they could use some refresh. Ultimately, all of these could be causes for the apparent predominance of the JS API for block registration.

mcsf commented 4 years ago

@emilio-martinez: you may want to keep an eye on #22151.

kuworking commented 3 years ago

A little hacky, but you can just conditionally set the default value:

const Editor = props => {
    const { setAttributes, attributes } = props;
    let { myAttribute } = attributes;
    if (!myAttribute) {
       setAttributes({ myAttribute: "foobar" });
   }
   return <div>{myAttribute}</div>;
}

registerBlockType("my/hack", {
  title: "Hack",
  icon: "list",
  category: "common",
  attributes: {
    myAttribute: {
      type: "string"
    }
  },
  edit: props => <Editor {...props} />,
  save: ({ attributes }) => <MyComponent {...attributes} />
});

Wouldn't this work?

registerBlockType('blah', {
  title: 'blah',
  category: 'blah',
  attributes: {
    att1: { type: 'string', default: '' },
    att2: { type: 'string', default: '' },
    att3: { type: 'string', default: '' },
  },
  edit: ({ attributes, setAttributes, className }) => {
    useEffect(() => {
      const initialAttributes = {
        att1: 'blah 1',
        att2: 'blah 2',
        att3: 'blah 3',
      }
      const defaultAttributes = {}
      for (const a in attributes) defaultAttributes[a] = attributes[a] || initialAttributes[a]
      setAttributes(defaultAttributes)
    }, [])
  //...
})
alexstandiford commented 3 years ago

I stumbled on this problem today. I'm inclined to pass the registered attributes from PHP to JS using a preloaded REST API endpoint.

I'm suggesting something like what's done here: https://github.com/WordPress/gutenberg/blob/459ab7fb1ad91907642e21a378fcef328edbf3cb/lib/edit-site-page.php#L151-L170

And add a REST API endpoint that retrieves the list of registered block attributes, as well. That could be merged in with the JavaScript's attributes object that's passed into registerBlockType as a spread operator.

naumanahmed19 commented 1 year ago

For a custom plugin, I am facing this issue, I am parsing blocks and using them as API, in that case, all default attributes are missing and there is no way to get values until the user touched or changes the value of the input

for example: When you add an image using core/image block, it displays image dimensions(height/width) in block settings but not in markup. Then it's confusing as well that there are values but not getting in HTML.

tcmulder commented 1 year ago

I'm experiencing this also trying to set a default backgroundColor value. Say I set the default in the block.json file:

{
...
"attributes": {
        "backgroundColor": {
            "type": "string",
            "default": "xyz"
        }
    }
}

In the editor, the appropriate class for the background color (i.e. "has-xyz-background-clor") gets applied, but it doesn't also get applied on the front end via get_block_wrapper_attributes.

What's more, if I do manually choose a different background color than the default and publish, the editor and get_block_wrapper_attributes both apply it properly, but if I then manually re-select the background color I have set as the default in block.json, it no longer gets added via get_block_wrapper_attributes.

As a result, not only am I unable to set a default background since it won't appear initially on the front-end, but also if I set a default background then that particular color is unable to be selected for the front end, manually or otherwise, though all other colors are selectable.

isuke01 commented 5 months ago

@tcmulder Here is another plot in the issue.

If you set default attribute like you did. Then If you try to disable Background colour - reset to no background colour for the block, it will seems to be correct in editor but this is not saved, if you try too refresh editor it will be back to default - which again, will not be applied on frontend :|

This apply to all kind of attributes not only BGColour.

MadtownLems commented 2 months ago

I'm quite surprised that the current state is the default behavior. I'm definitely in support of immediately inserting the appropriate default value attributes in the HTML comment markup for freshly inserted blocks. The current state, where the UI and editor will show colors and the like that aren't yet present in the post_content feels wrong.

BrennanButler commented 1 month ago

This is an incredibly annoying bug that I am also experiencing. I have also found that once a default value has been added to an attribute, removing it does not revert the behaviour to what you would expect - the attribute is still not found within the block attributes/markup.