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

Validation fails if attribute is persisted to post content & meta. #4989

Closed
2 tasks
Shelob9 opened this issue Feb 10, 2018 · 15 comments
Closed
2 tasks

Validation fails if attribute is persisted to post content & meta. #4989

Shelob9 opened this issue Feb 10, 2018 · 15 comments
Labels
[Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended
Milestone

Comments

@Shelob9
Copy link
Contributor

Shelob9 commented Feb 10, 2018

If a block saves markup with a representation of an attribute in HTML and that same attribute as meta, block validation fails. The meta is saved, the markup is good in post content, block validation fails.

Issue Overview

Consider this requirement for a block:

  1. Must save completely valid HTML markup to post content that, when the client has no JavaScript, is totally human and machine (assistive devices, search engines, etc) readable, and can be progressively enhanced by front-end JavaScript/CSS.
  2. The fields represented in that HTML markup should be queryable in the database.

Requirement 1 is provided by a Gutenberg static HTML block and requirement 2 is satisfied by the WordPress Meta API if the field is saved as post meta.

In my mind, saving an attribute as post meta, and static HTML representing its value in a semantic, human readable form is best for front-end requirements -- accessibility, no JS fallbacks, SEO, the philosophy of progressive enhancement -- and back-end needs -- we need to query by field data and update field data.

BTW I'm aware that this duplicates data. Given that we have a hook that runs after post meta is updated and we have a function for converting attributes to static HTML, that should be solvable.

Steps to Reproduce (for bugs)

  1. Register a block with a meta attribute and register the meta field
  2. Create block edit function so that you can edit attribute and see that setAttributes is updating state correctly.
    3.Create block save function that for a non-meta attribute .

Expected Behavior

No validation error.

Current Behavior

Block validation fails.

Possible Solution

I think the problem here is it's being parsed as if it's a an attribute being stored in markup/normal serialization, but I'm not sure. If so, can that be disabled in these situations?

Screenshots / Video

screen shot 2018-02-10 at 2 07 12 pm
screen shot 2018-02-10 at 2 07 30 pm

Related Issues and/or PRs

Todos

  • Tests
  • Documentation
@Shelob9
Copy link
Contributor Author

Shelob9 commented Feb 10, 2018

I created a workaround, but it involves a second "fake" content attribute, which is sub-optimal: https://gist.github.com/Shelob9/36d7096d919995ae615d4d4bbf10ffbc But, it serves as a proof of concept for this, which is cool.

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Feb 14, 2018
@youknowriad
Copy link
Contributor

youknowriad commented Feb 15, 2018

The problem is that the validation is executed while parsing the blocks, and the parsing doesn't know about the post object as it's supposed to be generic enough to be used in context with post information.

We might have to rethink adding specific block attributes sources like "meta" in a generic way where we could hook into the parser, the selector, the onChange etc... and enhance how these work.

cc @aduth

@aduth
Copy link
Member

aduth commented Feb 15, 2018

We might have to rethink adding specific block attributes sources like "meta" in a generic way where we could hook into the parser, the selector, the onChange etc... and enhance how these work.

Strange coincidence, I was iterating on an idea here last night.

Approach I'd thought was a filter in getBlockAttribute which editPost adds a hook for and allows custom source: "meta" handling.

@aduth
Copy link
Member

aduth commented Feb 15, 2018

One related issue I'd come across in starting this is that many of our post selectors are in editor, which ought to be migrated (especially as we plan to expose all of them).

@designsimply
Copy link
Member

Noting the console warning and error in plain text for reference:

Block validation: Expected token of type `EndTag`
Block validation: Block validation failed for `shelob9/metatest`

@aduth aduth self-assigned this Jul 9, 2018
@danielbachhuber danielbachhuber added the [Feature] Blocks Overall functionality of blocks label Oct 24, 2018
@danielbachhuber danielbachhuber added this to the WordPress 5.0 milestone Oct 24, 2018
@aduth
Copy link
Member

aduth commented Nov 16, 2018

While this is still acknowledged to be a bug, it's not one I'd considered a blocker for 5.0, given both the workaround and considering that the circumstances under which it would impact the vast majority of users approaches near impossible, if one considers that it would become plainly obvious to be broken to a block implementer in the course of minimal testing.

#5077 can serve as incomplete prior art toward a possible resolution, and may in-fact tie to some potential optimizations, given that the meta attribute computation in the getBlock selector (eliminated in #5077) has shown in recent performance audits to be a contributing factor to degraded experience.

@aduth
Copy link
Member

aduth commented Jan 31, 2019

Closed #13621 as a duplicate of this one. As noted there, this manifests itself by always producing an undefined value for a meta-sourced attribute in the save of a block implementation. The reason for this is that save is called during the initial parse of a block to verify block validity, but meta values are populated only after the blocks are parsed and loaded into the editor, since they are values extracted from the post being edited.

Noting also some recent related conversation to this point / issue in #13088 (#13088 (comment)).

@cbrakhage
Copy link

@aduth What is the work around to this? I'm a bit new to this, and I can't seem to figure out why my attribute isn't persisting after a page reload. It seems like initially my attribute isn't loaded, but when I delete the block and create a new one, both edit and save have the correct value.

@aduth
Copy link
Member

aduth commented Feb 12, 2019

@cbrakhage Have you looked over the code @Shelob9 had shared at #4989 (comment) ? It seems to involve duplicating the attribute value as saved in both the markup of a block and the post meta.

@Horttcore
Copy link

When will this be fixed? I mean this is some serious bug and makes meta attributes useless

@epiqueras
Copy link
Contributor

It looks like #16402 fixed this. I can't reproduce anymore.

@aduth
Copy link
Member

aduth commented Jul 12, 2019

@epiqueras If you're using the sample plugin I shared in #16402 as a base for testing, it doesn't include generated save output, so would not trigger the validation error observed here. In the sample, changing this line to return el( Stars, { stars: arguments[ 0 ].attributes.stars } ); should prompt the issue.

In order to resolve this issue, there are still changes necessary after #16402 to separate validation from the initial parse to account for the applied meta attributes.

@epiqueras
Copy link
Contributor

Strange, I did change it to return props.attributes.stars.

@aduth
Copy link
Member

aduth commented Jul 12, 2019

A fix is now proposed at #16569

@youknowriad
Copy link
Contributor

As of #17153, meta attribute values are no longer initialized into parsed blocks, which invalidates this approach. The decision here is that meta attributes are not meant to be used on save functions since save functions are supposed to be pure and the meta value can change dynamically outside of the block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
8 participants