Repository navigation
Excerpt block: remove filter hack, use excerpt_length - #69090
swissspidy wants to merge 13 commits into
Conversation
swissspidy
added
[Type] Bug
|
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: @claude. 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. |
This comment was marked as resolved.
This comment was marked as resolved.
| 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.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
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! 👍 |
Resolve conflicts in the Post Excerpt block: - edit.js: keep the `getEntityRecord` based data flow that passes the `excerpt_length` REST parameter, on top of trunk's import layout and `useDeprecatedTextAlign` refactor. - index.php: keep `block_core_post_excerpt_excerpt_length()`, which the render callback now uses, and drop the global `is_admin()` filter that the REST parameter makes unnecessary.
Dragging the "Max number of words" range control changed the `excerpt_length` query parameter on every intermediate value, firing a REST request for each step. Feed the request a debounced copy of the attribute instead. The range control and the client-side trimming keep reading the attribute directly, so dragging stays responsive while requests are only made once the value settles.
| const record = select( coreStore ).getEntityRecord( | ||
| 'postType', | ||
| postType, | ||
| postId, | ||
| { excerpt_length: requestedExcerptLength } | ||
| ); |
There was a problem hiding this comment.
is this code path newly exposed due to the PR changes?
getEntityRecord caches a single record by ID and context and ignores the rest of the query, so every requested excerpt_length shared one cache entry: the response for one length was displayed for another, and a length that had already been resolved was never requested again, leaving the block showing whichever length came back last. Request the generated excerpts outside that cache and key them by their request instead. Also skip the request entirely for posts that have a stored excerpt. wp_trim_excerpt() returns a stored excerpt untouched and applies the filter only while generating one from the content, so excerpt_length cannot change the response for those posts and the record the editor already loaded can be reused.
🤖 PR meta 🤖📦 Bundle sizeSize Change: +384 B (0%) Total Size: 8.21 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsShow the failuresSome 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. refuses the drop and uploads nothing in
|
|
Hey 👋 I wanted to give you a heads-up since this pull request is affected by recent validation changes for changelog files. #83043 adds additional validation for changelog files. You'll note that this pull request is currently failing a "Required changes from trunk" check. What you'll need to do: You will need to either rebase or merge the latest code from |
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