Excerpt block: remove filter hack, use excerpt_length
#69090
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @apeatling. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +74 B (0%) Total Size: 1.85 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Feb 7, 2025
Mamaduka
reviewed
Feb 7, 2025
Comment on lines
+58
to
+68
| const record = select( coreStore ).getEntityRecord( | ||
| 'postType', | ||
| postType, | ||
| postId, | ||
| { excerpt_length: excerptLength } | ||
| ); | ||
| const editedRecord = select( coreStore ).getEditedEntityRecord( | ||
| 'postType', | ||
| postType, | ||
| postId | ||
| ); |
There was a problem hiding this comment.
I still need to test my theory here, but have you checked this change with Query blocks? I think because of the new argument; selectors won't be able to use data from getEntityRecrods, which will trigger separate HTTP requests.
Not a big issue in separation, but it can degrade performance inside the loop.
There was a problem hiding this comment.
No I haven't tested this with the Query block. But a few extra HTTP requests in the editor seem unavoidable this way (frontend is not affected). And I am not here to fundamentally change how selectors or the Query block works, that would be way out of scope for this PR. But if there's any quick wins or so, I'm eager to learn more.
There was a problem hiding this comment.
And I am not here to fundamentally change how selectors or the Query block works, that would be way out of scope for this PR.
I wasn't suggesting that. I know it's not in this scope.
I'll try experimenting with it, but I can't promise anything until early next week.
P.S. We should simplify the data selector "orchestration" for consumers. Currently, improving performance for those requires familiarity with internals. It's okay for core but could be challenging for 3rd party consumers.
There was a problem hiding this comment.
Confirmed. Adding a new excerpt_length parameter triggers an additional request for each Excerpt block, which is not ideal, since it will increase editor loading time, but I guess it can't be avoided entirely.
Maybe we could mitigate this a bit by only applying the excerpt_length filter when excerptLength !== DEFAULT_EXCERPT_LENGTH.
|
Flaky tests detected in 2fe1b12. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13197175411
|
6 tasks
|
A shame that we were not able to land this in 6.8 😞 |
|
This still requires additional testing to avoid regressions, but it generally looks good. Some notes/questions:
cc @t-hamano |
|
I have confirmed that this PR works correctly in the Japanese locale, i.e. when the word count type is Here's what I noticed:
See the video below: 84faaead3a3158b8fc85221b4292ec9d.mp4 |
This looks to be a problem that still needs to be addressed. As the slider is dragged, lots of HTTP requests are triggered. Setting the excerpt length attribute could possibly be debounced. The RangeControl docs seem to specifically mention not to do that, but I'm not sure what the alternative is. There also seems to be a bug, in my testing after a while the slider stops having any effect. I'm not sure why, it might be an issue with entity caching when using query params. To repro keep changing the value between something high (90) and something low (10) and observe that after a while only one of the values sticks. |
|
What if we just make a single API request using I think the excerpt that is rendered inside the block could be calculated client-side only. const { rawExcerpt, renderedExcerpt, isProtected } = useSelect(
( select ) => {
const record = select( coreStore ).getEntityRecord(
'postType',
postType,
postId,
{ excerpt_length: 100 }
);
const editedRecord = select( coreStore ).getEditedEntityRecord(
'postType',
postType,
postId
);
return {
rawExcerpt:
record && editedRecord ? editedRecord.excerpt : null,
renderedExcerpt: record?.excerpt.rendered,
isProtected: record?.excerpt.protected,
};
},
[ postType, postId ]
);
// Based on rawExcerpt, renderedExcerpt, and excerptLength attribute, calculate the excerpt to be rendered in the block, client-side only.
// ... |
|
Trying to do the excerpt client-side would just reintroduce the same issues that were originally tried to solve, duplicate logic for multilingual cases, and most importantly would not use the server-side excerpt filters for an actual WYSIWYG experience. So 👎 from me.
Interesting. That would indeed indicate an issue at the entity caching layer. I don't think this can be solved as part of this PR. Dragging back and forth tons of times sounds like an edge case too. |
Even normal dragging can cause problems. See the video below: Even when dragging the slider from the minimum of 10 to the maximum of 100 at normal speed, there is still a mismatch between the set value and the actual length of the excerpt. This doesn't seem acceptable to me. 8dc2aac6fcc6776a41641d4b81706920.mp4I was wondering if there is any good idea to solve this problem while still reusing the server side logic on the client side. |
I tried this in #70790 and discovered that while debouncing fixes the excessive requests, the slider becomes visually unresponsive. Dragging the slider, it only pops into position after I stop dragging.
even dragging once causes a huge amount of requests that seems excessive. In this screencast I drag the slider once and see almost 160 requests: screencast.2025-07-18.11-03-05.mp4 |
|
I gave debouncing another try in #70793, aiming to only debouce the excerpt updating not the entire handler. This worked much better - the UI is responsive and a single drag results in requests after the drag. server requests are reduced by an order of magnitude 16 vs 160). screencast.2025-07-18.11-15-21.mp4wdyt @swissspidy? |
|
That looks smoother indeed! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #53570
Removes hacky PHP filter implementation that affects way more than just the REST API.
Uses the recommended
excerpt_lengthparameter that's available since WordPress 6.6Why?
Stop affecting unrelated workflows, fix bugs, write better code :)
How?
Uses
getEntityRecordinstead ofuseEntityPropso that custom query params can be passed.Testing Instructions
Testing Instructions for Keyboard
n/a
Screenshots or screencast
n/a