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

Try supporting post meta in block attributes #2740

Merged
merged 12 commits into from Sep 25, 2017

Conversation

Projects
None yet
5 participants
@mcsf
Contributor

mcsf commented Sep 16, 2017

Experimental. Not fit to merge. See updated docs for details.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 17, 2017

Codecov Report

Merging #2740 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2740      +/-   ##
==========================================
+ Coverage   33.77%   33.77%   +<.01%     
==========================================
  Files         189      191       +2     
  Lines        5640     5930     +290     
  Branches      982     1053      +71     
==========================================
+ Hits         1905     2003      +98     
- Misses       3163     3319     +156     
- Partials      572      608      +36
Impacted Files Coverage Δ
blocks/editable/index.js 10.31% <ø> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 97.91% <100%> (ø) ⬆️
editor/selectors.js 94.73% <78.57%> (-1.67%) ⬇️
editor/sidebar/post-schedule/index.js 63.15% <0%> (-3.51%) ⬇️
editor/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/sidebar/table-of-contents/item.js 0% <0%> (ø)
editor/sidebar/table-of-contents/index.js 0% <0%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68a93d4...d6f9296. Read the comment docs.

codecov bot commented Sep 17, 2017

Codecov Report

Merging #2740 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2740      +/-   ##
==========================================
+ Coverage   33.77%   33.77%   +<.01%     
==========================================
  Files         189      191       +2     
  Lines        5640     5930     +290     
  Branches      982     1053      +71     
==========================================
+ Hits         1905     2003      +98     
- Misses       3163     3319     +156     
- Partials      572      608      +36
Impacted Files Coverage Δ
blocks/editable/index.js 10.31% <ø> (ø) ⬆️
editor/modes/visual-editor/block.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 97.91% <100%> (ø) ⬆️
editor/selectors.js 94.73% <78.57%> (-1.67%) ⬇️
editor/sidebar/post-schedule/index.js 63.15% <0%> (-3.51%) ⬇️
editor/modes/visual-editor/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/sidebar/table-of-contents/item.js 0% <0%> (ø)
editor/sidebar/table-of-contents/index.js 0% <0%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68a93d4...d6f9296. Read the comment docs.

### Considerations
By default, a meta field will be excluded from a post object's meta. This can be circumvented by explicitly making the field visible:

This comment has been minimized.

@mtias

mtias Sep 20, 2017

Contributor

@pento this would be nice to address in general :)

@mtias

mtias Sep 20, 2017

Contributor

@pento this would be nice to address in general :)

This comment has been minimized.

@pento

pento Sep 21, 2017

Member

Yeah, that's annoying.

@rmccue: I don't recall the history for why postmeta is only included in a response when it has the show_in_rest flag set. (As opposed to at least returning the visible postmeta when the user is appropriately authenticated.)

@pento

pento Sep 21, 2017

Member

Yeah, that's annoying.

@rmccue: I don't recall the history for why postmeta is only included in a response when it has the show_in_rest flag set. (As opposed to at least returning the visible postmeta when the user is appropriately authenticated.)

This comment has been minimized.

@rmccue

rmccue Sep 21, 2017

Contributor

Meta fields aren't guaranteed to be "safe" by default. "Safe" is a few things, namely safe for JSON serialisation (which is lossy compared to PHP), and safe for capabilities (not all meta is always available).

The current behaviour exists for those reasons. When we initially didn't require show_in_rest, people noted that this would expose hidden fields created by plugin or users (turns out, a bunch of people use the "Custom Fields" metabox for random internal notes). Solving these problems in a consistent and safe way turned out to be essentially impossible.

show_in_rest is an indicator that plugin developers are accepting the limitations of the API for their meta fields.

@rmccue

rmccue Sep 21, 2017

Contributor

Meta fields aren't guaranteed to be "safe" by default. "Safe" is a few things, namely safe for JSON serialisation (which is lossy compared to PHP), and safe for capabilities (not all meta is always available).

The current behaviour exists for those reasons. When we initially didn't require show_in_rest, people noted that this would expose hidden fields created by plugin or users (turns out, a bunch of people use the "Custom Fields" metabox for random internal notes). Solving these problems in a consistent and safe way turned out to be essentially impossible.

show_in_rest is an indicator that plugin developers are accepting the limitations of the API for their meta fields.

This comment has been minimized.

@mtias

mtias Sep 21, 2017

Contributor

These hidden fields would be shown if you do get_post_meta() calls, no? It feels like the API should adhere to the expectations of doing such authenticated calls.

@mtias

mtias Sep 21, 2017

Contributor

These hidden fields would be shown if you do get_post_meta() calls, no? It feels like the API should adhere to the expectations of doing such authenticated calls.

Show outdated Hide outdated editor/selectors.js
@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 20, 2017

Contributor

This is looking good to me and opens up a lot of possibilities for block authors, specially those transitioning from meta-boxes.

Contributor

mtias commented Sep 20, 2017

This is looking good to me and opens up a lot of possibilities for block authors, specially those transitioning from meta-boxes.

@mtias

mtias approved these changes Sep 20, 2017

Show outdated Hide outdated docs/attributes.md

@mcsf mcsf merged commit 11864ff into master Sep 25, 2017

3 checks passed

codecov/project 33.77% (+<.01%) compared to 68a93d4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mcsf mcsf deleted the try/post-meta-support branch Sep 25, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 27, 2017

Member

We should have added some test cases for the revised getBlock behavior, specifically defining the expected behavior of the return value when the block not present in state, because it would have surfaced that it currently crashes the application.

  1. Select a block
  2. Press trash in secondary actions
  3. Boom
Member

aduth commented Sep 27, 2017

We should have added some test cases for the revised getBlock behavior, specifically defining the expected behavior of the return value when the block not present in state, because it would have surfaced that it currently crashes the application.

  1. Select a block
  2. Press trash in secondary actions
  3. Boom
}, {} );
// Avoid injecting an empty `attributes: {}`
if ( ! block.attributes && ! metaAttributes.length ) {

This comment has been minimized.

@aduth

aduth Sep 27, 2017

Member

I'm confused how this is working, since metaAttributes.length will always be undefined, and this will shortcut before merging in attributes. I assume this should have been Object.keys( metaAttributes ).length.

@aduth

aduth Sep 27, 2017

Member

I'm confused how this is working, since metaAttributes.length will always be undefined, and this will shortcut before merging in attributes. I assume this should have been Object.keys( metaAttributes ).length.

This comment has been minimized.

@aduth

aduth Sep 27, 2017

Member

Also, shouldn't we still want to merge meta attributes even if there are no other attributes of the block?

@aduth

aduth Sep 27, 2017

Member

Also, shouldn't we still want to merge meta attributes even if there are no other attributes of the block?

This comment has been minimized.

@mcsf

mcsf Sep 28, 2017

Contributor

I'm confused how this is working

Ah, this one fell through the cracks. :( At some point there was a size( metaAttributes ).

merge meta attributes even if there are no other attributes of the block?

👍

@mcsf

mcsf Sep 28, 2017

Contributor

I'm confused how this is working

Ah, this one fell through the cracks. :( At some point there was a size( metaAttributes ).

merge meta attributes even if there are no other attributes of the block?

👍

return result;
}, {} );
// Avoid injecting an empty `attributes: {}`

This comment has been minimized.

@aduth

aduth Sep 27, 2017

Member

Is there a negative impact of injecting an empty attributes object? I think I might rather we normalize on the empty object.

@aduth

aduth Sep 27, 2017

Member

Is there a negative impact of injecting an empty attributes object? I think I might rather we normalize on the empty object.

This comment has been minimized.

@mcsf

mcsf Sep 28, 2017

Contributor

The intent was to preserve the original behavior (returning an untouched item from blocksByUid) unless there was any meta. Due to my lack of knowledge, this seemed preferable, in case any consumer of the selector was comparing object references.

@mcsf

mcsf Sep 28, 2017

Contributor

The intent was to preserve the original behavior (returning an untouched item from blocksByUid) unless there was any meta. Due to my lack of knowledge, this seemed preferable, in case any consumer of the selector was comparing object references.

This comment has been minimized.

@aduth

aduth Sep 29, 2017

Member

The intent was to preserve the original behavior (returning an untouched item from blocksByUid) unless there was any meta. Due to my lack of knowledge, this seemed preferable, in case any consumer of the selector was comparing object references.

Yeah, the intent became clearer in #2805 putting together the changes there, and I left the existing behavior. 👍

@aduth

aduth Sep 29, 2017

Member

The intent was to preserve the original behavior (returning an untouched item from blocksByUid) unless there was any meta. Due to my lack of knowledge, this seemed preferable, in case any consumer of the selector was comparing object references.

Yeah, the intent became clearer in #2805 putting together the changes there, and I left the existing behavior. 👍

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jan 30, 2018

Member

How does one use a meta attribute value in a (server-rendered) dynamic block?

Member

aduth commented Jan 30, 2018

How does one use a meta attribute value in a (server-rendered) dynamic block?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 31, 2018

Contributor

@aduth wouldn't that be just get_post_meta( $id, $attr_key ) on the server?

Contributor

mtias commented Jan 31, 2018

@aduth wouldn't that be just get_post_meta( $id, $attr_key ) on the server?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Jan 31, 2018

Member

Wouldn't one expect this to be available in the $attributes passed to render_callback ?

Member

aduth commented Jan 31, 2018

Wouldn't one expect this to be available in the $attributes passed to render_callback ?

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Jan 31, 2018

Contributor

I think there will likely be that expectation, yes. For the first pass, it seemed acceptable to require fetching via get_post_meta.

Mind you, this availability of data in $attributes doesn't happen for other non-comment sources (DOM, mainly); a somewhat similar example, wp:block, also handles post retrieval from ref. I mention this so we can better frame the discussion of which and how attributes should be provided to render_callback.

Contributor

mcsf commented Jan 31, 2018

I think there will likely be that expectation, yes. For the first pass, it seemed acceptable to require fetching via get_post_meta.

Mind you, this availability of data in $attributes doesn't happen for other non-comment sources (DOM, mainly); a somewhat similar example, wp:block, also handles post retrieval from ref. I mention this so we can better frame the discussion of which and how attributes should be provided to render_callback.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Feb 12, 2018

Contributor

Not sure if expecting from attributes is better than using the standard API for retrieving meta fields — which is more likely to have been used by a theme already.

Contributor

mtias commented Feb 12, 2018

Not sure if expecting from attributes is better than using the standard API for retrieving meta fields — which is more likely to have been used by a theme already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment