WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#39232 closed defect (bug) (fixed)

REST API: Can't set existing post format if theme does not support it (2)

Reported by: iseulde Owned by: jnylen0
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description

Follow up ticket on #38916.

The schema for the post format depends on the theme. Should it be?

Some highlights:

Replying to danielbachhuber:

As a corollary example to consider, what should happen if you try to update template for a Page to a template value that doesn't exist in the theme?

Replying to joehoyle:

We only recently changed the validation to only support the values allowed by the current theme (as that's broadly what the admin does), however I thinking maybe that was a mistake and we should always support _all_ the post formats in the API. Even if we fixed the validation issue, it's somewhat annoying that you can get a post in the API which has an post format that you, as an api client, never knew the existence of.

Replying to Drivingralle:

I think the behavior to just allow the post formats of the current theme is bad for a long term use of user data.

The current theme maybe doesn't support the format "aside" but the theme used two month later does.
If a user publishes the same content using a client via REST API suddenly gets different results. Because the newer posts suddenly have the format "aside" and the older once are "standard".

If we always allow to set the default post formats a client can write the full set of information into the post. If a theme leverages the data or not.
This way we make it pain-free to switch themes, something we should aim for.

We also allow to post comments into the API even if the theme doesn't show/support them.

Attachments (2)

39232.diff (4.0 KB) - added by JPry 3 years ago.
Changes with theme support being ignored
39232.2.diff (3.3 KB) - added by jnylen0 3 years ago.
Modify the existing "unsupported post format" tests instead of removing them.

Download all attachments as: .zip

Change History (24)

#1 @mark-k
3 years ago

+1. the behavior of post formats was always broken for anyone that thought about using them. Replicating the broken behavior might be consistent but isn't it better to fix the core issue? If I have a post with one image the post format I should set is an image whether the theme I currently use does something special for it or not.

In the context of the rest api - I can use the internal API to change the post format if the theme supports it or not, why shouldn't I be able to do it from the rest api?

#2 @jnylen0
3 years ago

Replying to @iseulde from ticket:38916#comment:5

Shouldn't you be able to POST the data you got back to the server without errors?

I think this is an excellent design principle for an API. Cases where this is not possible often point to problems in the API or elsewhere.

Proposed solution for this ticket:

  • Always allow the standard post formats to be set; show them as part of the format enum in the arguments and schema.
  • Additionally allow the current post format to be set, even if the theme does not support it. This should be a no-op.

I'm fine with addressing this in the API interface first, then addressing the larger issue in core (as @Drivingralle explained) separately.

#3 follow-up: @iseulde
3 years ago

Replying to joehoyle:

I think the distinction here is site wide conditional (say for the theme) or conditional based off a specific post.

As far as I understand, this is the main argument against the change, right? That it would be introducing conditional logic that is specific to the post? The API isn't consistent with this either though. From what I saw in the post controller, there is password logic that depends on the sticky value.

Replying to iseulde:

Shouldn't you be able to POST the data you got back to the server without errors?

On second thought, this would have much bigger consequences. When you have an invalid thumbnail ID, what should happen? Or are we then differentiating between values that depend on the theme rather than the database?

#4 in reply to: ↑ 3 ; follow-up: @Drivingralle
3 years ago

When you have an invalid thumbnail ID, what should happen? Or are we then differentiating between values that depend on the theme rather than the database?

An invalid thumbnail ID would lead to broken data and needs to be prevented. No theme or plugin can work with a wrong/broken post_thumbnail.

But a post_format not used by the current theme, but supported by core is not a wrong/broken piece of information. It's just not used at this point in time.

If a theme adds a custom post_format the API should validate the value correct. And after a theme switch the format is gone because it's part of the theme. The default formats are still inside WP.

#5 @Drivingralle
3 years ago

Is there anything I can do to bring this ticket forward?

#6 @jnylen0
3 years ago

We need consensus on an approach, and we need a patch with unit tests. I still think the approach I outlined in comment:2 is sensible.

#7 in reply to: ↑ 4 @JPry
3 years ago

Replying to jnylen0:

Proposed solution for this ticket:

  • Always allow the standard post formats to be set; show them as part of the format enum in the arguments and schema.
  • Additionally allow the current post format to be set, even if the theme does not support it. This should be a no-op.

jnylen0:

We need consensus on an approach, and we need a patch with unit tests. I still think the approach I outlined in comment:2 is sensible.

I think I'm +1 to your first bullet point. However, I don't agree with the second bullet point. Instead, I agree with this:

Drivingralle:

But a post_format not used by the current theme, but supported by core is not a wrong/broken piece of information. It's just not used at this point in time.

I think it makes sense to still set the post format, even if the theme doesn't support their use. If a theme isn't using post formats, or isn't using certain kinds of post formats, then it should just ignore them.

However, I disagree with this portion from Drivingralle:

If a theme adds a custom post_format the API should validate the value correct. And after a theme switch the format is gone because it's part of the theme. The default formats are still inside WP.

Themes aren't supposed to add their own custom post formats. By extension, I don't think that the API should support any "custom" post formats. Although, if others decided that it should, then we could put a filter in place where the post formats are added to the enum field.

@JPry
3 years ago

Changes with theme support being ignored

#8 @JPry
3 years ago

  • Keywords has-patch 2nd-opinion has-unit-tests added

#9 follow-ups: @jnylen0
3 years ago

Themes aren't supposed to add their own custom post formats.

I wasn't aware of this, but on its own isn't a good enough reason to remove support.

Do themes add post formats anyway? I'm not sure; we'll need someone who's more familiar with this feature to weigh in.

This ticket was mentioned in Slack in #core-themes by jnylen. View the logs.


3 years ago

#11 in reply to: ↑ 9 @Drivingralle
3 years ago

Replying to jnylen0:

Do themes add post formats anyway? I'm not sure; we'll need someone who's more familiar with this feature to weigh in.

Themes can announce that they support a post format.
https://codex.wordpress.org/Post_Formats#Adding_Theme_Support

The info like icon and labels for the format are included inside core.

#12 in reply to: ↑ 9 ; follow-up: @davidakennedy
3 years ago

Replying to jnylen0:

Themes aren't supposed to add their own custom post formats.

I wasn't aware of this, but on its own isn't a good enough reason to remove support.

Do themes add post formats anyway? I'm not sure; we'll need someone who's more familiar with this feature to weigh in.

Responding to the post in Core Themes Slack.

Themes cannot add "custom" post formats. See: https://codex.wordpress.org/Post_Formats

New formats cannot be introduced by themes or even plugins.

As for point two in 2:

Additionally allow the current post format to be set, even if the theme does not support it. This should be a no-op.

I'd say I agree with this:

But a post_format not used by the current theme, but supported by core is not a wrong/broken piece of information. It's just not used at this point in time.

Because if you use post formats and switch themes, that data – what format it is still exists. A theme declaring it is just the theme's decision because it's usually connected to a front end display difference.

But I can imagine API clients wanting to set and show that data differently. To some degree, they could be "themes" in a loose sense, as in not a traditional WordPress theme, but still wanting to use that data in a creative way.

That's just my take. Let me know if I missed anything, or you need anything else.

#13 in reply to: ↑ 12 @jnylen0
3 years ago

  • Keywords needs-refresh added; 2nd-opinion removed
  • Owner set to jnylen0
  • Status changed from new to accepted

Replying to davidakennedy:

Themes cannot add "custom" post formats. See: https://codex.wordpress.org/Post_Formats

New formats cannot be introduced by themes or even plugins.

This is the piece I was missing here, thank you. There is a big difference between "should not" and "cannot" :)

Given that, I agree with the approach in 39232.diff. I don't think we should remove the tests though - let's make sure the requests succeed instead.

#14 @jnylen0
3 years ago

  • Milestone changed from Awaiting Review to 4.8

#15 @Drivingralle
3 years ago

Does it make sense to include this patch into 4.7.3?

This way currently beeing developed API-clients don't need to implement soon superfluous checks. Other API misbehavior are also changed in that minor release as far as I understood.

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


3 years ago

#17 @jnylen0
3 years ago

  • Milestone changed from 4.8 to 4.7.3
  • Status changed from accepted to assigned

Thanks for the reminder, @Drivingralle. I had forgotten about this ticket. I'll try to land it for 4.7.3 today.

@jnylen0
3 years ago

Modify the existing "unsupported post format" tests instead of removing them.

#18 @jnylen0
3 years ago

  • Keywords commit added; needs-refresh removed
  • Status changed from assigned to accepted

This is good to go with 39232.2.diff.

#19 @jnylen0
3 years ago

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

In 40120:

REST API: Allow setting post formats even if they are not supported by the theme.

A post_format not used by the current theme, but supported by core is not a wrong/broken piece of information. It's just not used at this point in time. Therefore we should allow setting and retrieving any of the standard post formats supported in core, even if the current theme doesn't use them.

After this commit, a post's format value can survive a round trip through the API, which is a good general design principle for an API.

Props JPry, iseulde, davidakennedy, Drivingralle.
Fixes #39232.

#20 @jnylen0
3 years ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 4.7 branch. Edit: Two commits to backport - [40120] and [40121].

Last edited 3 years ago by jnylen0 (previous) (diff)

#21 @jnylen0
3 years ago

In 40121:

REST API: Update the client test fixtures after changes to post formats.

This commit updates the wp-api-generated.js fixture file after recent changes to the way post formats work in the API.

See #39232.

#22 @ocean90
3 years ago

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

In 40137:

REST API: Allow setting post formats even if they are not supported by the theme.

A post_format not used by the current theme, but supported by core is not a wrong/broken piece of information. It's just not used at this point in time. Therefore we should allow setting and retrieving any of the standard post formats supported in core, even if the current theme doesn't use them.

After this commit, a post's format value can survive a round trip through the API, which is a good general design principle for an API.

Merge of [40120] and [40121] to the 4.7 branch.

Props JPry, iseulde, davidakennedy, Drivingralle.
Fixes #39232.

Note: See TracTickets for help on using tickets.