Fix inconsistencies in excerpt length when doing AJAX requests vs regular HTTP requests vs REST API requests

[anton-vlasenko]

What?

This PR addresses inconsistencies in excerpt length when making AJAX requests versus regular HTTP requests and REST API requests.

Fixes #53570.

Why?

When attempting to load posts via AJAX requests, the excerpt length is always set to 100 words. This issue arose because the previous implementation incorrectly initialized the filter, assuming that it needed to be initialized when WP_ADMIN === true. Consequently, the previous implementation would override the excerpt length when making AJAX requests via admin-ajax.php, leading to the problem with excerpt length.

How?

Now, the excerpt length is only overridden when making REST API requests, which is the intended behavior.

Testing Instructions

Steps to test this PR:

1. Do not check out this branch yet. Let's reproduce the bug first.
2. Activate a block theme, such as Twenty Twenty-Four.
3. Ensure that Gutenberg is enabled.
4. Add the following code to the functions.php file in your theme's folder:

function custom_excerpt_length( $length ) {
	return 20;
}

add_filter( 'excerpt_length', 'custom_excerpt_length', 1667 );
function get_latest_post() {
	$args = array(
		'post_type'      => 'post',
		'posts_per_page' => 1,
		'orderby'        => 'date',
		'order'          => 'DESC',
	);

	$latest_post = new WP_Query( $args );

	if ( $latest_post->have_posts() ) {
		while ( $latest_post->have_posts() ) {
			$latest_post->the_post();
			echo get_the_excerpt();
		}
		wp_reset_postdata();
	} else {
		echo 'No posts found.';
	}

	die();
}

add_action( 'wp_ajax_nopriv_get_latest_post', 'get_latest_post' );
add_action( 'wp_ajax_get_latest_post', 'get_latest_post' );

5. Build Gutenberg with npm run build to ensure that the Gutenberg's build/ directory has the correct version of the hook.
6. Create a new POST in the Site Editor. You can use any title. Use the following text as the post's content (just make sure to insert it into a paragraph block):

Lorem ipsum dolor sit amet, consectetur adipiscing elit. In euismod diam egestas massa facilisis, in imperdiet mi consequat. Etiam interdum vel tellus laoreet posuere. Aliquam ut sagittis elit. Nullam et orci nisl. Morbi quis vehicula ex, eget luctus odio. Vivamus a tincidunt arcu, id euismod ex. Praesent fringilla velit lorem, sit amet commodo mauris vulputate sodales. Vestibulum tempus, ipsum et ultricies interdum, ligula elit accumsan risus, vel tristique dolor est eu diam. Vivamus pharetra eget erat vel rutrum. Aliquam et ligula quis lorem molestie semper. Aenean convallis tortor nec velit ultricies semper. Maecenas felis erat, cursus nec varius et, auctor eu nulla. Phasellus et nibh imperdiet, placerat erat ut, tristique libero. Nam sed scelerisque enim.

7. Open the Site Editor (wp-admin/site-editor.php) page.
8. Execute the following code in the console of your browser:

jQuery.ajax( {
    url: wpApiSettings.root + 'wp/v2/posts?per_page=1&orderby=date&order=desc',
    method: 'GET',
    beforeSend: function ( xhr ) {
        xhr.setRequestHeader( 'X-WP-Nonce', wpApiSettings.nonce );
    }
} ).done( function ( response ) {
    console.log( response[0].excerpt.rendered );
} );

9. Observe the response. Note that the excerpt length is 100 words, as expected, since the excerpt length in REST responses should be 100 words.

10. Execute the following code in the console of your browser:

jQuery.ajax( {
    url: '/wp-admin/admin-ajax.php', // Assuming your WordPress installation is in the web root of your server.
    method: 'POST',
    data: {
        action: 'get_latest_post'
    }
} ).done( function ( response ) {
    console.log( response );
} );

11. Observe the response. Notice that the excerpt length is 100 words, even though the custom_excerpt_length hook in the functions.php file should limit the length to 20 words. The bug is confirmed.

12. Check out this PR.

13. In both src/wp-includes/blocks/post-excerpt.php and build/wp-includes/blocks/post-excerpt.php, replace:

if ( is_admin() ||
	defined( 'REST_REQUEST' ) && REST_REQUEST ) {
	add_filter(
		'excerpt_length',
		static function () {
			return 100;
		},
		PHP_INT_MAX
	);
}

with:

add_filter(
	'excerpt_length',
	static function ( $value ) {
		// This check needs to be inside the callback since the REST_REQUEST constant
		// is not defined at the time add_filter() is called.
		if ( ! ( defined( 'REST_REQUEST' ) && REST_REQUEST ) ) {
			return $value;
		}

		return 100;
	},
	PHP_INT_MAX
);

14. Build Gutenberg with npm run build to ensure that the Gutenberg's build/ directory has the correct version of the hook.
15. Repeat step 8. Ensure that the excerpt length is still 100 words in the response.
16. Repeat step 10. Ensure that the excerpt length is 20 words in the response, just as specified in the custom_excerpt_length hook. The bug is fixed.

[github-actions]

Flaky tests detected in f8a8839.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7637438979
📝 Reported issues:

• [Flaky Test] Only the first block is an empty paragraph block #58161 in /test/e2e/specs/editor/blocks/paragraph.spec.js

[anton-vlasenko]

The testing instructions have been updated.

[swissspidy]

The whole approach is still not great because the excerpt will be limited to 100 for any sort of REST request, even when not in the editor. Ideally this should only be added in edit context in the REST API. Plus. plugins cannot easily unhook it because it's an anonymous function added at PHP_INT_MAX.

[swissspidy]

Also, unit tests would be great.

[azaozz]

plugins cannot easily unhook it because it's an anonymous function added at PHP_INT_MAX

Yea, I know this is preexisting code but seems it would be good to remove the PHP_INT_MAX filter priority "nonsense" as it is a bad way to do things in WP, especially in core.

Think that comes from some plugins trying to be "too clever" about adding their callback after everybody else. Unfortunately that doesn't work as any other plugin can use PHP_INT_MAX too and make sure it is loaded after the plugin that is trying to be clever :)

Imho core should never use something like that, it's just a bad way to use filters. If something must run after all the filters have finished running, it can be hard-coded after the apply_filters() call. If not, a priority of 20 should be sufficient, no matter what.

[azaozz]

The whole approach is still not great because the excerpt will be limited to 100 for any sort of REST request

Yea, this is the preexisting code. Enforces 100 for all excerpts that are used through REST. That seem to be needed in the editor only. Comes from #44964.

Seems this is sub-optimal. The (unresolved) question seems to be "Who has priority to set the excerpt length?" The user (in the editor) or a plugin that the user has enabled?

In any case the enforcing of 100 for excerpt length through REST should probably be removed by fixing/updating how that works for REST requests/in the editor. Easiest seems to add an override (to bypass setting the length) that is used only for REST edit context as @swissspidy suggests above.

[anton-vlasenko]

Thank you for the feedback, @swissspidy and @azaozz.

I hope I've addressed all of your concerns, including adding unit tests and lowering the prioirty to 20.

I'd appreciate your approval and/or another code review. Thank you.

P.S. I know that that the unit test fails on some old PHPUnit versions. I don't have the time to fix it now. I'll fix that in the next couple of hours.

[swissspidy]

I'm afraid I fail to understand why you are calling the current approach hacky.

Because it won't work for cases like REST API preloading or batch requests, where it could still have unintended side effects.

but doesn't your proposed code apply the get_the_excerpt and the_excerpt filters twice

No, as we are getting the $post->post_excerpt again from scratch

[anton-vlasenko]

Because it won't work for cases like REST API preloading or batch requests

You are right. Well spotted! Thank you. I've changed the code so now it should work with preloading and batch requests. I've tested it with preloading and it works, but it should also work for batch requests too.

No, as we are getting the $post->post_excerpt again from scratch

I have two concerns about the proposed solution, which are just my personal opinion:

1. The solution completely rewrites the excerpt field, ignoring any controller-specific code. This might cause issues with custom third-party REST controllers that might have their own business logic for handling the excerpt field.

2. It applies filters twice (although I understand that we are getting $post->post_excerpt from scratch), which seems wrong to me from the architectural standpoint. Also, it might lead to performance issues.

Therefore, in this PR, I decided not to "overwrite" the excerpt field.

[swissspidy]

Hooking into rest_pre_dispatch without checking whether it's actually a relevant request is way too broad again. You don't want to override the excerpt length for every single post ever returned in the REST API when in edit context.

At this point, it feels like we're riding in circles here. I recommend you to ask REST API maintainers in the REST API Slack channel for advice.

I suppose a more bullet proof way of filtering the excerpt length only when relevant is by introducing some sort of new query argument to force the limit to be applied. Something like ?force_excerpt_length=100 or so.

[azaozz]

I'm starting to think this may need a bit larger changes. I like @swissspidy's idea to add another query argument to set the excerpt length.

If not, perhaps can add another filter specifically for this. Seems that would work too?

Also seems that adding either a query arg or a filter will be useful in other cases too.

[anton-vlasenko]

On Friday, I updated the PR to make it more selective and only add the filter when there is an "excerpt" field in the response (borrowing this idea from your code snippet). I'm not able to come up with anything better that doesn't rewrite the excerpt field.

I recommend you to ask REST API maintainers in the REST API Slack channel for advice.

Thank you, I've taken that advice into account. Honestly, I doubt there is another "hook" that would allow us not to overwrite the excerpt field and at the same time provide fine-grained control over the exact post type to which the filter should be applied.

But I will check that.

At this point, it feels like we're riding in circles here.

Haha. I have the same feeling. I will leave implementing the solution with force_excerpt_length to another contributor, as I'm running out of steam.

I really appreciate your feedback, @swissspidy.

[Rajinsharwar]

Hi @anton-vlasenko, I guess @swissspidy is right. After using this patch, the AJAX call is correct showing the length defined in the "excerpt_length" hook. But, when the call is made via the REST API, it shows 100 words, which shouldn't be the case, as our aim is to make it consistent.

[Rajinsharwar]

Just sent out a new PR to solve both cases. Requesting a review @hellofromtonya @swissspidy.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/tests/blocks/registerBlockCorePostExcerptLengthFilter.php

[anton-vlasenko]

Hi @anton-vlasenko, I guess @swissspidy is right. After using this patch, the AJAX call is correct showing the length defined in the "excerpt_length" hook. But, when the call is made via the REST API, it shows 100 words, which shouldn't be the case, as our aim is to make it consistent.

Thank you for the test report, @Rajinsharwar . I've addressed that.

[annezazu]

@anton-vlasenko what's needed to move this forward and have it considered for 6.4.3? cc @aaronjorbin as a heads up since I see you're helping with 6.4.3 triage for Core.

[draganescu]

I rebased this.

[draganescu]

I checked out this PR and tried the instructions but I failed to make it work with steps 13-16.

Insetad I modified the value in register_block_core_post_excerpt_length_filter to 20 in this PR and got this result which looks ok to me:

• the ajax request has the default 100 length
• the editor rest request has the specified 20 length

@swissspidy I tried to follow the conversation in the code comments, but I couldn't grok if you want to halt this PR in its tracks or if you think this would be a good patch to follow up on? Am I correct in saying that this approach solves one problem while potentially creating others?

The suggested approach with a special query param should take more time than we have for the next release and going that route will likely get the ticket punted again. That is no reason to rush things, but just worth mentioning. I do think that instead of doing this weird filter dance being clear in saying:

hey API I have this special case where I need to override the site option

... is better. Or maybe even update the post excerpt block to set the global option (like site title sets the site title).

Giving this more thought:

• there is a global excerpt length setting
• the post excerpt block offers users the ability to override it locally for certain loops
• which works on the front end by on the fly changing the setting whien rendering the block 👏🏻
• but fails in the editor
• so we use filters to hack the rest response - which inadvertently always bleeds into unrelated rest operatios

The above sounds to me like we should really drop all filters and just handle it client side for the UX requirement to show excerpts in the editor the length set by the user on the block. So .. IDK get the full text and make the excerpt in the browser?

[swissspidy]

@draganescu The current solution is brittle and will likely create more problems than it solves.

The suggested approach with a special query param should take more time than we have for the next release and going that route will likely get the ticket punted again.

I am happy to steward such a change for 6.5. In fact, I just opened WordPress/wordpress-develop#5932 to do this as it's a very simple enhancement.

IMHO this is the most robust way to do this.

The above sounds to me like we should really drop all filters and just handle it client side for the UX requirement to show excerpts in the editor the length set by the user on the block

The risk here is that the client-side solution will not match the server-side generated excerpt, either because the algorithm is slightly different from core's or because some plugins override the logic.

That's why I think a REST API param is more robust.

[draganescu]

@swissspidy I looked at your PR in WordPress/wordpress-develop#5932 and I think we can close this in favor of that.

Your PR solves the problem in the sense that what the excerpt block does is change the excerpt length for it's local representation only - wherever it's used with a custom value. That is handled at render so in the editor adding that extra param to the REST request is precisely what is going on - much more robust that trying to do it via filters applied on every request that leak the setting - the setting should only work in the context of the excerpt block.

[draganescu]

Oops, I'll leave the closing to the author if @anton-vlasenko agrees, didn't mean to actually close it :)

[anton-vlasenko]

What's needed to move this forward and have it considered for 6.4.3?

@annezazu, I was AFK when you tagged me, and then I lost the notification somewhere in the depths of the GitHub user interface. :( I'm sorry.

And I think we can close this in favor of that.

Yes, I agree, @draganescu.

I like @swissspidy's idea to add an extra parameter to the request, which he implemented in WordPress/wordpress-develop#5932. IMO, doing something explicitly is almost always better than fiddling with conditions and trying to "guess" when the excerpt length needs to be overridden (the solution that existed before).

IMHO, this is the most robust way to do this.

I agree. Personally, I don't mind if it takes more time because this solution, from an architectural standpoint, is better than the one that existed before @swissspidy's PR.

Closing this in favor of WordPress/wordpress-develop#5932.