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

Fix: Excerpt limit is overruled (#48403) #48598

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

tmanhollan
Copy link

What?

REST_REQUEST constant was mistakenly quoted, which caused the condition to always evaluate to true. And the conditional logic should be AND, not OR.

Why?

Issue #48403 describes the problem. Excerpt length is overridden always, rather than just in the editor as intended.

How?

Fixes the conditional logic so implementations of excerpt_length filter hook will only be overridden in the block editor.

Testing Instructions

  1. Implement the following hook:
add_filter( 'excerpt_length', function( $excerpt_length ) {
	return 1000;
}, 100 );
  1. Add or edit a post in the block editor.
  2. Use the post-excerpt block.

Testing Instructions for Keyboard

n/a

Screenshots or screencast

n.a

REST_REQUEST constant was mistakenly quoted, which caused the condition to always evaluate to true. And the conditional logic should be AND, not OR.
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 28, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @tmanhollan! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@skorasaurus skorasaurus added the [Block] Post Excerpt Affects the Post Excerpt Block label Feb 28, 2023
@skorasaurus
Copy link
Member

Thanks for the quick pull request and initial report.

This PR did fix the issue of the excerpt_length filter being ignored in my PHP templates ; however in the editor, the post excerpts block's content length is limited to the upper bound of the excerpt_length filter. For example, if you have excerpt_length filter set to 55 but in the block editor, set the block's excerpt to 100; the excerpt will only display 55 words in the editor... if you change block's excerpt to 25 in the editor; the block will correctly display 25 words.

@tmanhollan
Copy link
Author

Thank you, @skorasaurus, you're right.

I think there are two problems: primarily, the condition is not being evaluated in the correct context. It should be inside the callback; in the outer scope I think it's happening before REST_REQUEST is set. Additionally, it appears is_admin() isn't true for the rest request that returns the post(s). In fact, I don't think that even needs to be part of the condition, but I'm not sure if there's some other case where it might be true.

I'm making that revision now. But I don't think REST_REQUEST being true is sufficient on its own for knowing that it's being loaded in the editor. Is there more to consider there? I really don't know that much about this API; I just discovered the bug while trying to figure out why my excerpt length was being ignored.

@skorasaurus
Copy link
Member

skorasaurus commented Mar 1, 2023

Thanks; with your latest commit, the post_excerpt block was now working again within the editor although someone else had caught it and fixed it (#48654) but they didn't wrap it up in a callback. I haven't checked yet if their commit, already merged into trunk fixes all of these issues.

@skorasaurus
Copy link
Member

skorasaurus commented Mar 1, 2023

Query loop block:

(using #48654)
when:
excerpt_length hook is set to 50
excerpt length block's attribute set to 85
what displays in editor: value of excerpt_length hook
what displays in front end: value of excerpt_length hook

excerpt_length hook is set to 50
excerpt length block's attribute set to 25
what displays in editor: excerpt length block's attribute
what displays in front end: excerpt length block's attribute


for #48598 as of https://github.com/WordPress/gutenberg/blob/21367410f67674fd5ecfe9d0280c42c62d81daf4/packages/block-library/src/post-excerpt/index.php

excerpt_length hook is set to 50
excerpt length block's attribute set to 85
what displays in editor: excerpt length block's attribute
what displays in front end: value of excerpt_length hook

excerpt_length hook is set to 50
excerpt length block's attribute set to 25
what displays in editor: excerpt length block's attribute
what displays in front end: excerpt length block's attribute

@tmanhollan
Copy link
Author

I think I see the problem in the front end. It's not loaded via REST like it is in the editor, so the condition in the filter is not satisfied (the only reason it was originally is that the condition always returned true because of the quoted constant). I think we need another excerpt_length filter applied in render_block_core_post_excerpt right before get_the_excerpt is called to make sure the starting value isn't trimmed shorter than the max that can be set in the block's setting. I'll try that out now.

The filter that is conditional upon REST_REQUEST does not apply when the block content is rendered in the front end. This added filter serves the same purpose when rendering the block for the front end.
@tmanhollan
Copy link
Author

Ok I think this last revision makes the front end and editor consistent and excerpts that are not part of a block obey the value returned by the excerpt_length hook.

excerpt_length hook is set to 50
excerpt length block's attribute set to 85
displayed in the editor: value of attribute (85)
displayed in the front end: value of attribute (85)
regular excerpt on another page (not part of the post-excerpt block): value of of hook (50)

excerpt_length hook is set to 50
excerpt length block's attribute set to 25
displayed in the editor: value of attribute (25)
displayed in the front end: value of attribute (25)
regular excerpt on another page (not part of the post-excerpt block): value of of hook (50)

@skorasaurus
Copy link
Member

skorasaurus commented Mar 3, 2023

(For anyone new coming to this: even though the original issue #48403 is closed; this should be kept open to determine whether this is an improvement of #48654; also see review at #44964 (review) in #44964

@carolinan
Copy link
Contributor

I am not able to do the code review, I am not confident I would get it right since I caused the bugs last time.
From the comment it does look like this is an improvement following the smaller fix, but the solution for the last filter is different, I can't determine which is the right solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Excerpt Affects the Post Excerpt Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants