Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#39996 closed defect (bug) (fixed)

Allow passing existing template value for posts, even when template does not exist

Reported by: danielbachhuber's profile danielbachhuber Owned by: westonruter's profile westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-unit-tests has-patch commit
Focuses: Cc:

Description

In r40120, it became possible for a client to set format=>aside on a post, even when when the theme doesn't support aside as a post format.

Under this same logic, it should be possible for a client to set template=>my-file.php, even when my-file.php isn't a file in the theme.

The current behavior is that it's not possible for a client to set template to an arbitrary file name (values are validated against an enum).

Related #39232

Attachments (2)

39996.diff (5.2 KB) - added by TimothyBlynJacobs 7 years ago.
39996.2.diff (5.3 KB) - added by TimothyBlynJacobs 7 years ago.

Download all attachments as: .zip

Change History (33)

#1 follow-up: @jnylen0
8 years ago

  • Keywords needs-unit-tests added

This is pretty different than post formats - the current theme may not support all formats, but there is still a finite list of possibly supported formats.

If we apply the same logic for templates, we are essentially saying that clients can set any arbitrary data (or just an arbitrary PHP filename) in this field, which doesn't seem right. Instead, I think the change we should make here is to enable setting a post to the same template value that it currently has - this should be a no-op even if the current theme doesn't support that value.

#2 in reply to: ↑ 1 @dd32
8 years ago

Replying to jnylen0:

If we apply the same logic for templates, we are essentially saying that clients can set any arbitrary data (or just an arbitrary PHP filename) in this field, which doesn't seem right. Instead, I think the change we should make here is to enable setting a post to the same template value that it currently has - this should be a no-op even if the current theme doesn't support that value.

I agree with this - There doesn't appear to be any value in allowing it to be set to a value which isn't supported by the theme - as it's theme-specific data (unlike post formats), however we shouldn't force a client to wipe out the existing value in order to resave a post.
@danielbachhuber If you've got a use-case, I'd love to hear it

#3 @danielbachhuber
8 years ago

Instead, I think the change we should make here is to enable setting a post to the same template value that it currently has - this should be a no-op even if the current theme doesn't support that value.

This seems like a reasonable decision to me.

@dd32 I don't have a specific use case, other than the original presented: a client POSTing back the same data it received.

#4 follow-up: @swissspidy
8 years ago

It's a bit of a mess it seems.

WP_REST_Posts_Controller::update_item() calls wp_update_post(), which doesn't allow you to update the post's template to the current value. If you'd change that, the meta entry would get updated twice because the update_item() method calls WP_REST_Posts_Controller::handle_template().

Ideally, we fix wp_update_post(), remove the call to handle_template() and add a new test for this behaviour.

#5 in reply to: ↑ 4 ; follow-up: @jnylen0
8 years ago

Replying to swissspidy:

Ideally, we fix wp_update_post(), remove the call to handle_template() and add a new test for this behaviour.

Couldn't we just unset $data['template'] from the update if it's the same as the current value? This would skip both passing it to wp_update_post and calling handle_template.

the meta entry would get updated twice because the update_item() method calls WP_REST_Posts_Controller::handle_template().

It sounds like this is probably already happening during any normal post update, no?

#6 in reply to: ↑ 5 @jnylen0
8 years ago

Replying to jnylen0:

It sounds like this is probably already happening during any normal post update, no?

Upon a closer look, we never pass page_template to wp_update_post, so its logic for updating a page template is never triggered by the API.

So this looks like a pretty straightforward change to me, only touching WP_REST_Posts_Controller::handle_template. @swissspidy do you agree or is there something I'm missing here?

#7 follow-up: @swissspidy
8 years ago

So this looks like a pretty straightforward change to me, only touching WP_REST_Posts_Controller::handle_template. @swissspidy do you agree or is there something I'm missing here?

IIRC this fails much earlier in the schema validation because of the enum. It never gets to this method.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

#9 in reply to: ↑ 7 @jnylen0
8 years ago

Replying to swissspidy:

IIRC this fails much earlier in the schema validation because of the enum. It never gets to this method.

We can solve this with a similar approach to e.g. check_username in 38739.diff. I'll get a patch posted here soon.

#10 @jnylen0
8 years ago

  • Owner set to jnylen0
  • Status changed from new to assigned

#11 @rmccue
8 years ago

  • Summary changed from Grant clients more liberty in setting 'template' attribute to Allow passing existing template value for posts, even when template does not exist

Renaming the ticket to better match the decision here.

#12 @jnylen0
8 years ago

  • Milestone changed from 4.7.4 to 4.7.5

Punting to 4.7.5.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


8 years ago

#14 @desrosj
8 years ago

  • Milestone changed from 4.7.5 to 4.8

#15 @jnylen0
8 years ago

  • Milestone changed from 4.8 to 4.8.1

I'm going to punt this again as it's not super high priority and there is no patch yet.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

#17 @jbpaul17
8 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


7 years ago

#19 @TimothyBlynJacobs
7 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

Attached a patch to use validate_callback as suggested.

I also had to account for the built in validation that wp_insert_post() performs for the page template. When updating the post, the original template value gets merged into the update data. This means that even if the page template was validated accounting for updating to the same value, internal validation would prevent it from being saved.

To account for this, I set page_template to null so that template handling will be bypassed in wp_insert_post.

Since the template is now validated at request time, I removed the extra validation in ::handle_template() by default. However, I kept it when performing a create request because the filter on the templates can be contextual based on the new post object and we don't have the new post ID when validating at request time. It might be worth returning an error in this case as well, similar to updating terms, or meta, etc...

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


7 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


7 years ago

#24 @westonruter
7 years ago

  • Keywords needs-patch added; has-patch removed

@TimothyBlynJacobs I think the enum should be removed entirely in favor of completely using the validate_callback as the available templates are dynamic.

It turns out that the list of available page templates for a given page is not necessarily a subset of all of the page templates in a theme. A plugin can dynamically make extra page templates available for certain pages, being sourced from plugins. As an example, see from Elementor. For more, see discussion on Gutenberg.

#25 @TimothyBlynJacobs
7 years ago

That makes sense, I'll update.

#26 @TimothyBlynJacobs
7 years ago

  • Keywords has-patch added; needs-patch removed

Added patch that removes enum.

#27 @danielbachhuber
7 years ago

+1 to the overall approach in 39996.2.diff — a dynamic callback works fine for this need.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#29 @westonruter
7 years ago

  • Keywords commit added
  • Owner changed from jnylen0 to westonruter
  • Status changed from assigned to accepted

I'll take the +1 from @danielbachhuber as vote for commit.

#30 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 41979:

REST API: Allow passing existing template value for posts even when template no longer exists.

Also remove enum for validating allowed templates to allow plugins to dynamically supply their own templates for specific posts, even when they are not in the theme.

Props TimothyBlynJacobs, jnylen0, swissspidy.
Fixes #39996.

#31 @pento
7 years ago

In 42032:

Tests: Update wp-api-generated.js.

[41979] caused a change in wp-api-generated.js, so it needs to be updated.

See #39996.

Note: See TracTickets for help on using tickets.