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

REST API: allow overriding excerpt length #5932

Closed

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Jan 23, 2024

Addresses WordPress/gutenberg#53570 on the core side

Related:

Trac ticket: https://core.trac.wordpress.org/ticket/59043


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@azaozz
Copy link
Contributor

azaozz commented Jan 23, 2024

The code seems okay but I'm still unsure why this has to work in this particular way. Generally this change makes it so sometimes the excerpt length may be overridden with an arbitrary value when doing a REST request. Couple of thoughts about that:

  • Seems the initial need was for a hard-coded override of 100 that is only for use in the editor and is later trimmed from JS. Not really sure why this was changed to an arbitrary value as that doesn't seem needed?
  • The change seems implemented in a somewhat strange way by adding a non-removable filter callback. As it is still a filter, the excerpt length value can be overridden by adding another filter callback that runs after this one. However is seems that the block editor is (somewhat) hard-coded to expect the exact length of 100 words for the excerpt in edit context, so allowing that to be overridden doesn't seem like a particularly good idea?

How about instead of adding that non-removable filter callback we add another param like $override_length = null to wp_trim_excerpt() and pass that through the 'get_the_excerpt' filter that's used right underneath? So

$excerpt = apply_filters( 'get_the_excerpt', $post->post_excerpt, $post );

would become:

$excerpt = apply_filters( 'get_the_excerpt', $post->post_excerpt, $post, (int) $override_excerpt_length );

where $override_excerpt_length may actually be better off as a hard-coded 100. Untested, but thinking this would work perhaps a bit better? At least will enforce the length when set through the REST API and perhaps prevent "strange" accidental edge cases in the future if the excerpt_length filter is used by a plugin that "must enforce its own setting at all costs" :)

@swissspidy
Copy link
Member Author

However is seems that the block editor is (somewhat) hard-coded to expect the exact length of 100 words for the excerpt in edit context [...]

How did you get to this conclusion, where did you get this number from? 🤔

The whole issue right now is that Gutenberg offers a way to fully customize the excerpt length in the editor, but that length is not actually honored in the REST API response it receives.
Screenshot 2024-01-23 at 23 49 21

This PR is an attempt to solve this in a less hacky way than WordPress/gutenberg#55400

As it is still a filter, the excerpt length value can be overridden by adding another filter callback that runs after this one.

Theoretically, yes. With PHP_INT_MAX this risk is greatly reduced though.

But plugins can also completely override the excerpt using the get_the_excerpt filter anyway. There's no safeguard for that.

How about instead of adding that non-removable filter callback we add another param like $override_length = null to wp_trim_excerpt() and pass that through the 'get_the_excerpt' filter that's used right underneath? So
At least will enforce the length when set through the REST API and perhaps prevent "strange" accidental edge cases in the future if the excerpt_length filter is used by a plugin that "must enforce its own setting at all costs" :)

This approach does not prevent a plugin from filtering get_the_excerpt and enforcing its own setting at all costs.

So in this regard, both approaches have the same problem and I don't think it's solvable. Plugins will always be able to filter the excerpt.

@azaozz
Copy link
Contributor

azaozz commented Jan 23, 2024

How did you get to this conclusion, where did you get this number from? 🤔

The initial bug report, here, and the initial patch? :)

The whole issue right now is that Gutenberg offers a way to fully customize the excerpt length in the editor, but that length is not actually honored in the REST API response it receives.

May be missing something but how would that slider work if the excerpt was too short? Lets say the user sets the excerpt length to 20, how can they increase it next time they edit the post? Afaik this is the problem here, the editor needs a consistent excerpt length (100?) that can be trimmed on display to allow the user to change it (and actually see it).

This approach does not prevent a plugin from filtering get_the_excerpt

Right. But that most likely would be a deliberate change targeting exactly this functionality. I'm thinking more about accidental overrides by plugins that do filters with PHP_INT_MAX, etc.

@swissspidy
Copy link
Member Author

The initial bug report, WordPress/gutenberg#53570, and the initial patch? :)

There was no need for a hardcoded excerpt length. That was simply a hacky workaround to provide something to the editor to work with. 100 is the highest number that can be chosen in the editor. No proper solution was implemented which actually returns the actually configured length.
So I am offering this PR to fix that, as it allows trimming excerpts at the desired maximum length.

May be missing something but how would that slider work if the excerpt was too short?

Just like the excerpt_length filter, the whole setting in the editor is a maximum excerpt length. If your post_content is shorter, you'll get a shorter excerpt.

Lets say the user sets the excerpt length to 20, how can they increase it next time they edit the post?

The excerpt block is used in places like Query Loop blocks. If you want to increase the excerpt length in places where you used the block, go edit the block.

But questions about the excerpt block are better target at someone who built the block :)
I'm merely offering help here to fix a bad implementation.

Afaik this is the problem here, the editor needs a consistent excerpt length (100?) that can be trimmed on display to allow the user to change it (and actually see it).

What the editor needs is a way to trim the excerpt length to the desired maximum and this change here allows it to do so. At WordPress/gutenberg#55400 (comment) I explained why it's better to do the trimming server-side rather than client-side.

@draganescu
Copy link

draganescu commented Jan 24, 2024

We also need a Gutenberg PR to use this new param. @azaozz I think this approach is the best since it solves the actual problem: one block wants excerpt to be size X for itself when it renders and when it displays. It should not do this in any way that has any potential to leak the setting to other operations involving excerpt.

Having a special param gives the block this option. I also think that the fact that in the end WP can be configured in ways which make this param not work as expected is something to be dealt with at the end of the line - at that implementation: e.g. if your excerpt modifying plugin breaks how you work with the block deal with the plugin.

Plugins themselves can also check the request and not override it so it's easy to adapt.

Copy link

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good to me.
I have some minor suggestions.

Co-authored-by: Mukesh Panchal <[email protected]>
Copy link

github-actions bot commented Feb 5, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props swissspidy, antonvlasenko, mukesh27, azaozz, andraganescu, timothyblynjacobs.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@WordPress WordPress deleted a comment from moshiajonta Feb 5, 2024
@swissspidy
Copy link
Member Author

@azaozz I would appreciate another review

If we can merge this now into core there is still time to get the corresponding change into this week's Gutenberg RC so we can finally fix this in 6.5

@TimothyBJacobs
Copy link
Member

This is a solution that I don't love, but can't think of anything better.

The reason I don't love it, is it feels different to how we handle everything else that is variable (like image sizes, avatar sizes, raw vs rendered) where the API spits out multiple choices, and the client chooses the one that is closest to their desired size. I think this is the first time we're giving the client control over a specific API property like that.

But that all being said, I can't really think of a better way of handling this. And I think this can be thought of as an extension to things like ?_fields.

@draganescu
Copy link

With the latest thumbs up this is definitely ready IMO :)

@swissspidy
Copy link
Member Author

Committed in https://core.trac.wordpress.org/changeset/58065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants