Use `targetSchema` of JSON Hyper Schema to communicate sticky action #6529

danielbachhuber · 2018-05-01T21:49:27Z

Description

Updates the post-sticky check to use presence of wp:action-sticky in _links to determine whether the sticky UI should display. This system is called targetSchema. It allows us to compute whether a user can perform an action server-side, and then communicate it to the client.

See #6361 (comment)

How has this been tested?

For editors and above, the "Stick to the Front Page" UI should display:

For authors and below, it should not:

User Switching is a helpful plugin for quickly testing this.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

kadamwhite · 2018-05-02T00:09:01Z

editor/components/post-sticky/test/index.js

+			<PostStickyCheck postType="page" post={ {
+				_links: {
+					self: {
+						href: 'https://w.org/wp-json/wp/v2/posts/5',


Should this be /page/5 if postType="page"? (Doesn't change test result, admittedly)

kadamwhite · 2018-05-02T00:21:03Z

editor/components/post-sticky/test/index.js

+					},
+					'wp:action-sticky': {
+						href: 'https://w.org/wp-json/wp/v2/posts/5',
+					},


Once again the test works as-is because of what we're testing for, but all of the keys on the _links object hold arrays of objects, instead of plain objects
(e.g. wp:action-sticky': [ { /* ... */ } ]).

kadamwhite · 2018-05-02T00:23:37Z

lib/rest-api.php

+							),
+						),
+					),
+				),


We have this issue all over the API, but this is a lot of data to be piling into the response just to indicate whether the UI can take some action. Do we really want to be repeating all of this:

for each post every time we're in a collection where we might possibly want to sticky something? Our _links object is getting quite heavy.

I don't see a better option so I'm 👍 on the implementation, but I think we'll need to come back to that at some point.

If we ever land generating a URL to access a schema, the targetSchema section could be replaced with a $ref for the property.

We could also remove the description property but it'd make the link less helpful for clients.

gziolo · 2018-05-02T07:14:11Z

editor/components/post-sticky/check.js

@@ -27,14 +23,8 @@ export function PostStickyCheck( { postType, children, user } ) {
 export default compose( [
 	withSelect( ( select ) => {
 		return {
+			post: select( 'core/editor' ).getCurrentPost(),


It would be better to return only:

hasStickyAction: get( post, [ '_links', 'wp:action-sticky' ], false )`

from withSelect to avoid unnecessary rerenders when the current post changes after unrelated modifications.

In general, it is recommended to pass down as props only what is really used inside the component.

Good catch, fixed in 3e591c5

Because WordPress' capabilities system is filterable, we need to execute the capabilities before we know whether the current user can perform the action. Fortunately, `targetSchema` supports exactly this use-case.

felixarntz · 2018-05-03T09:20:34Z

lib/rest-api.php

+	// Only Posts can be sticky.
+	if ( 'post' === $post->post_type ) {
+		if ( current_user_can( $post_type->cap->edit_others_posts )
+			&& current_user_can( $post_type->cap->publish_posts ) ) {


Why is the capability for publishing posts required to make a post sticky?

More importantly though, this should use the meta capabilities for the specified post and rely on map_meta_cap() instead of handling the logic manually:

if ( 'post' === $post->post_type ) { if ( current_user_can( 'edit_post', $post->ID ) && current_user_can( 'publish_post', $post->ID ) { // Add link. } }

Why is the capability for publishing posts required to make a post sticky?

I'm using the check within edit_post() as my point of reference: https://github.com/WordPress/WordPress/blob/3266b10d04ad43b9d983e0b55d4d6247be33e18a/wp-admin/includes/post.php#L401

More importantly though, this should use the meta capabilities for the specified post and rely on map_meta_cap() instead of handling the logic manually.

I'm not sure I follow. Can you explain why?

I'm using the check within edit_post() as my point of reference: https://github.com/WordPress/WordPress/blob/3266b10d04ad43b9d983e0b55d4d6247be33e18a/wp-admin/includes/post.php#L401

Okay, I didn't know this was previously being used for it. I'm not sure that check makes sense though even in this area.
Basically, any action you perform to a specific item (like a post) should go through a meta capability check (using a singular noun, like edit_post or publish_post). Logic to resolve these to their primitive required capabilities is in place in the map_meta_cap() function, and developers rely on this functionality, for example to revoke that permission from a user (for example for one specific post).
Therefore I think the check in edit_post() is not accurate either. For this very case, there's no appropriate capability yet, so I'd recommend to introduce stick_post and unstick_post meta capabilities and add a clause for them in map_meta_cap(). We can then resolve it to the primitive capabilities we want to in there (if those are decided to be what currently happens in edit_post(), that's alright I guess). The benefit is that we then don't have to memorize all over what to check for when making a post sticky or unsticky, we can simply use intuitive stick_post / unstick_post, both in the current backend, the REST API, anywhere.

Therefore I think the check in edit_post() is not accurate either. For this very case, there's no appropriate capability yet, so I'd recommend to introduce stick_post and unstick_post meta capabilities and add a clause for them in map_meta_cap(). We can then resolve it to the primitive capabilities we want to in there (if those are decided to be what currently happens in edit_post(), that's alright I guess). The benefit is that we then don't have to memorize all over what to check for when making a post sticky or unsticky, we can simply use intuitive stick_post / unstick_post, both in the current backend, the REST API, anywhere.

Does what you describe need to be solved for in this pull request?

Given that only the post post type can be sticky, I don't think there's a need to add these special meta capabilities.

pento

I've been letting this tumble around for a bit, and I have a few concerns.

First off, I like the idea of providing information in the form of "this is an action the current user can perform, and here are the properties that action relates to". This is useful information to provide.

As has been mentioned, this solution seems quite verbose. I guess a major part of that is the description being on the post object, instead of in the schema, where all the other descriptions are. It's also duplicating the description of the sticky property, rather than documenting what action-sticky is for.

On that note, the naming is quite obtuse, too, it took me way too long to understand how the data is supposed to be used. There's nothing to indicate that "the presence of wp:action-sticky mean you should render the Sticky Post UI".

I think this would be better in a sibling property to _links, _actions, or something like that. _links is entirely used as a list of shortcuts to related information about the current object, adding in "here is an action the current user can perform" is confusing.

pento · 2018-05-04T04:29:36Z

lib/rest-api.php

+	// Only Posts can be sticky.
+	if ( 'post' === $post->post_type ) {
+		if ( current_user_can( $post_type->cap->edit_others_posts )
+			&& current_user_can( $post_type->cap->publish_posts ) ) {


Given that only the post post type can be sticky, I don't think there's a need to add these special meta capabilities.

kadamwhite · 2018-05-04T08:38:11Z

I think this would be better in a sibling property to _links, _actions, or something like that. _links is entirely used as a list of shortcuts to related information about the current object, adding in "here is an action the current user can perform" is confusing.

This would have the added benefit that such information could be omitted using _fields when the client does not need to know about actions

danielbachhuber · 2018-05-04T11:48:02Z

I think this would be better in a sibling property to _links, _actions, or something like that. _links is entirely used as a list of shortcuts to related information about the current object, adding in "here is an action the current user can perform" is confusing.

Can you point me to the existing spec where this is documented?

pento · 2018-05-04T11:56:56Z

Can you point me to the existing spec where this is documented?

It isn't documented as far as I'm aware, this is just how _links seems to be used in the core REST API. It's suggested by the name, too: a link is something you click (or in HTTP verbs, something you GET), it's not a thing that you POST.

As far as adhering to the JSON API spec goes, I'm fine with using something that exists within the spec, if that fits our requirements. If not, I'm 💯 in favour of doing something that works, rather than something that's shoe-horned into a vaguely related structure.

danielbachhuber · 2018-05-04T12:15:38Z

It isn't documented as far as I'm aware, this is just how _links seems to be used in the core REST API.

_links is an implementation of HAL. targetSchema is a part of the current JSON Hyper-Schema spec.

If not, I'm 💯 in favour of doing something that works, rather than something that's shoe-horned into a vaguely related structure.

I'm not particularly interested in spending a ton of time bikeshedding an alternative spec. Can you describe the alternative implementation you'd accept?

cc @TimothyBJacobs for any thoughts he has.

pento · 2018-05-04T12:45:51Z

Sure, here's a rough idea of something I think would be extensible, would clearly show the difference between _links and _actions.

In the schema, _actions would be defined as an array of all the possible actions, with a description of what they're for, and what they're related to, like so:

{
	"routes": {
		"/wp/v2/posts/(?P<id>[\\d]+)": {
			"endpoints": [
				{
					"methods": [ "POST", "PUT", "PATCH" ],
					"args": {
						// ...
					},
					"_actions": {
						"wp:sticky": {
							"title": "Sticky Post",
							"description": "Whether the current user can sticky the post or not",
							"targetSchema": {
								// Something in here about being related to the "sticky" property.
							}
						}
					}
				},
			],
		},
	},
}

Then, in the post object, the _actions property would define whether each action is available or not:

{
	"id": 1,
	// ...
	"_actions": {
		"wp:sticky": true,
	},
}

Alternatively, _actions could be an array of available actions, the difference seems to mostly be a choice of style:

{
	"id": 1,
	// ...
	"_actions": [
		"wp:sticky",
	],
}

danielbachhuber · 2018-05-04T12:57:09Z

Again, I'm strongly opposed to inventing a new spec when the existing spec is sufficient. In inventing a new spec, we'll need to go through multiple iterations of relearning past mistake/decision cycles. This doesn't seem like a judicious use of our time.

To address response size concerns, what if we only included these action links if context=edit?

TimothyBJacobs · 2018-05-04T14:43:38Z

I agree with @danielbachhuber. I don't see why including them in a separate "magic" field makes much sense. As I mentioned earlier, we can definitely omit the description fields which would considerably reduce the response size.

We've talked in #core-restapi a few times about adding links to the actual schemas. We could revisit this and try to land it for 5.0. This would allow us to replace the entire property description with a $ref like "$ref": "https://example.org/wp-json/wp/v2/?_method=OPTIONS&context=schema#/properties/sticky"

I also think including the links only if the context=edit makes a lot of sense as well. However, these can be added to the $schema document like this:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "title": "post",
  "type": "object",
  "properties": {
    "date": {
      "description": "The date the object was published, in the site's timezone.",
      "type": "string",
      "format": "date-time",
      "context": [
        "view",
        "edit",
        "embed"
      ]
    }
  },
  "links": [
    {
      "rel": "https://api.w.org/action-sticky",
      "href": "https://actualwebsite.com/wp-json/wp/v2/posts/{id}",
      "targetSchema": {
        "type": "object",
        "properties": {
          "sticky": {
            "$ref": "#/properties/sticky"
          }
        }
      }
    }
  ]
}

Our response could possibly then look something like.

{
  "_links": {
    "self": [],
    "wp:action-sticky": [
      {
        "href": "https://w.org/wp-json/wp/v2/posts/5"
      }
    ]
  }
}

However, we are not currently using this pattern anywhere else and we are introducing concepts like URI Templates and JSON Pointers, which while I'd be in favor of the API supporting, are not things we have looked at before.

pento · 2018-05-05T12:52:14Z

To address response size concerns, what if we only included these action links if context=edit?

Yah, that'd be fine. I don't think there's a need for them in other contexts, we can always add it to them if there is. That's not really the issue, though.

The specific concern I have is that, when reading these new links, there's nothing to suggest what they mean, or how you use them. The same problem exists in @TimothyBJacobs' example: there's nothing to suggest what wp:action-sticky means, someone might guess it's related to the https://api.w.org/action-sticky thing in the schema, but I wouldn't rely on that. Even if they do, there's still nothing to tell them what it actually means.

The problem we need to solve is that there are pieces of UI that should only be displayed when a certain set of conditions are met: we want to be calculating those conditions on the server, rather than leaving it up to the client. targetSchema is nice, but it's a super complex way making available what are essentially a list of flags.

So, here's what I would like to see:

The post object returning a list of these flags.
The schema containing a description of the list, and of each flag.

_actions happens to be the name I gave that list in my earlier example, but I have no attachment to that: if there's a spec that more readily fits this, then use that. If targetSchema can be adjusted to do those things, then use it.

If there isn't a spec, then we can just make something that works, and iterate it on it later if needed.

danielbachhuber · 2018-05-05T15:54:08Z

@pento I'm not sure I follow your specific recommendation. What do you think about cce57b8? Or, if there's other changes you'd like to make, can you communicate them in a pull request against this branch?

TimothyBJacobs · 2018-05-05T16:55:29Z

I disagree. If you understand what targetSchema means, then it is immediately obvious what is going on. Gutenberg is filled with dozens of these technical hurdles that aren't immediately obvious to WordPress developers until you understand what a HoC is, or Slot & Fill, etc... The same thing is true with a number of patterns in WordPress core as well.

I also disagree that it is super complex. Its a fairly pattern of representing what the target resource looks like and is certainly no more complex than many other G7g patterns that are necessary to fulfill a complex requirement.

danielbachhuber · 2018-05-05T18:30:41Z

To clarify my own position: I'm open to being convinced there is an implementation superior to that proposed on this pull request. However, at this point, I'm unclear as to what the alternative implementation is, and how it's functionally better.

From my perspective, the targetSchema implementation on this pull request is correct because:

It conveys the information needed by the client.
It follows an existing, documented spec that much of the REST API already follows.

TimothyBJacobs · 2018-05-05T19:47:57Z

In case it wasn’t clear, I of course have no issue with some other mechanism being used to indicate what actions are available, but I very much do not want to see WordPress adopt another standard in a weird way that isn’t really applicable and break the purpose of it being standard without a very good reason. And I don’t think that targetSchema is complicated enough to warrant that breakage.

pento · 2018-05-07T01:54:44Z

Thank you for the discussion, @danielbachhuber and @TimothyBJacobs. cce57b8 is a step in a good direction. I've opened #6613 with some small tweaks, but I think we can go with this.

…6613)

We're not introducing meta caps at this point.

danielbachhuber · 2018-05-07T12:31:39Z

Thanks @pento.

rickalee · 2024-07-26T16:17:28Z

What is the best method to remove 'Sticky' option/toggle from Posts globally now?

danielbachhuber added this to the 2.8 milestone May 1, 2018

danielbachhuber requested review from pento and a team May 1, 2018 21:49

danielbachhuber mentioned this pull request May 1, 2018

Ability to select authors has vanished #3010

Closed

kadamwhite approved these changes May 2, 2018

View reviewed changes

gziolo reviewed May 2, 2018

View reviewed changes

Use targetSchema of JSON Hyper Schema to communicate sticky action

e2bf660

Because WordPress' capabilities system is filterable, we need to execute the capabilities before we know whether the current user can perform the action. Fortunately, `targetSchema` supports exactly this use-case.

danielbachhuber force-pushed the 6361-post-sticky branch from 18e815f to e2bf660 Compare May 2, 2018 13:47

danielbachhuber added 3 commits May 2, 2018 06:49

Ensure links are properly formatted

67f0f9b

Avoid re-renders when the current post changes

3e591c5

Technically more correct test

53ce334

danielbachhuber requested a review from gziolo May 2, 2018 13:53

Update tests for 3e591c5

8c5330f

felixarntz previously requested changes May 3, 2018

View reviewed changes

aduth modified the milestones: 2.8, 2.9 May 3, 2018

pento reviewed May 4, 2018

View reviewed changes

Only include wp:action-sticky for context=edit; move description

cce57b8

pento mentioned this pull request May 7, 2018

Tweak targetSchema Response #6613

Merged

Move the description text from the targetSchema to the link title. (#…

3a9ccc1

…6613)

danielbachhuber merged commit 4dc4c7b into master May 7, 2018

danielbachhuber deleted the 6361-post-sticky branch May 7, 2018 12:31

felixarntz mentioned this pull request Jul 26, 2018

Capabilities Improvements cedaro/satispress#70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `targetSchema` of JSON Hyper Schema to communicate sticky action #6529

Use `targetSchema` of JSON Hyper Schema to communicate sticky action #6529

danielbachhuber commented May 1, 2018 •

edited

Loading

kadamwhite May 2, 2018

danielbachhuber May 2, 2018

kadamwhite May 2, 2018 •

edited

Loading

danielbachhuber May 2, 2018

kadamwhite May 2, 2018

TimothyBJacobs May 2, 2018

TimothyBJacobs May 2, 2018

gziolo May 2, 2018 •

edited

Loading

danielbachhuber May 2, 2018

felixarntz May 3, 2018 •

edited by danielbachhuber

Loading

danielbachhuber May 3, 2018

felixarntz May 3, 2018 •

edited

Loading

danielbachhuber May 3, 2018

pento May 4, 2018

pento left a comment

pento May 4, 2018

kadamwhite commented May 4, 2018

danielbachhuber commented May 4, 2018

pento commented May 4, 2018

danielbachhuber commented May 4, 2018

pento commented May 4, 2018

danielbachhuber commented May 4, 2018

TimothyBJacobs commented May 4, 2018 •

edited

Loading

pento commented May 5, 2018

danielbachhuber commented May 5, 2018

TimothyBJacobs commented May 5, 2018

danielbachhuber commented May 5, 2018

TimothyBJacobs commented May 5, 2018

pento commented May 7, 2018

danielbachhuber commented May 7, 2018

rickalee commented Jul 26, 2024

Use targetSchema of JSON Hyper Schema to communicate sticky action #6529

Use targetSchema of JSON Hyper Schema to communicate sticky action #6529

Conversation

danielbachhuber commented May 1, 2018 • edited Loading

Description

How has this been tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamwhite May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz May 3, 2018 • edited by danielbachhuber Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamwhite commented May 4, 2018

danielbachhuber commented May 4, 2018

pento commented May 4, 2018

danielbachhuber commented May 4, 2018

pento commented May 4, 2018

danielbachhuber commented May 4, 2018

TimothyBJacobs commented May 4, 2018 • edited Loading

pento commented May 5, 2018

danielbachhuber commented May 5, 2018

TimothyBJacobs commented May 5, 2018

danielbachhuber commented May 5, 2018

TimothyBJacobs commented May 5, 2018

pento commented May 7, 2018

danielbachhuber commented May 7, 2018

rickalee commented Jul 26, 2024

Use `targetSchema` of JSON Hyper Schema to communicate sticky action #6529

Use `targetSchema` of JSON Hyper Schema to communicate sticky action #6529

danielbachhuber commented May 1, 2018 •

edited

Loading

kadamwhite May 2, 2018 •

edited

Loading

gziolo May 2, 2018 •

edited

Loading

felixarntz May 3, 2018 •

edited by danielbachhuber

Loading

felixarntz May 3, 2018 •

edited

Loading

TimothyBJacobs commented May 4, 2018 •

edited

Loading