Repository navigation
add filter for target content id, improve elementor related ids detection (WP-1020) - #637
vsolovei-smartling wants to merge 5 commits into
Conversation
… (WP-1020) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…d (WP-1020) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…y (WP-1020) - TargetIdFilter: tolerate any argument type, never throw, cache lookups in the object cache, log misses and ambiguity at notice, return invalid source id unchanged when falling back - Elementor query exclude_term_ids/exclude_ids are remapped to translated ids but no longer submitted for translation (Content::isRemapOnly) - validate Elementor query ids as integers, move loop widget related content into ElementorQueryRelated::addLoopRelated - add tests Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sl-mmuradov
left a comment
sl-mmuradov
left a comment
There was a problem hiding this comment.
Thanks, the direction looks good. Replacing the trait with the composed ElementorQueryRelated removes the duplicated code in Posts and Loop Carousel, and the remap-only flag on Content is small and backward compatible.
Blocking
- Term ID type (please verify): as far as I know, Elementor Pro stores query term IDs as
term_taxonomy_ids, while we resolve and remap them asterm_ids. On sites where they differ, this submits the wrong terms and writes broken IDs back, which is the WP-1020 symptom itself. This existed before forinclude_term_ids, but the PR extends it to more settings and widgets. - Leftover query settings: IDs are collected regardless of the widget's query mode. Since
posts_idsis now submitted, an old hidden manual selection can queue many posts. Please check{prefix}post_type/{prefix}include/{prefix}excludefirst.
Should fix
normalizeReferences()overwrites taxonomy references instead of merging, which drops detected query terms when the document has its own terms in that taxonomy. Fix here or file a follow-up.- Document that excluded IDs are only remapped if the content is already translated when the translation is applied.
Notes on code not changed by this PR
inc/Smartling/Services/ContentRelationsDiscoveryService.php:576-578:$result[$taxonomy] = $ids;overwrites the result instead of merging. If an Elementor document has terms of its own in a taxonomy (e.g. a post with categories), any query terms from the same taxonomy that this PR detects are dropped from the related list. Suggest$result[$taxonomy] = array_merge($result[$taxonomy] ?? [], $ids);, either here or in a follow-up ticket.inc/Smartling/ContentTypes/Elementor/ElementAbstract.php:138: for query settings,getTerm()here receives aterm_taxonomy_id, not aterm_id(see the comment onElementorQueryRelated.php:22).tests/Mocks/WordpressFunctionsMockHelper.php:212:term_taxonomy_id => 0means the tests can't tellterm_idandterm_taxonomy_idapart. Worth giving it a different value once the ID type is handled.
Note: I couldn't run the test suite locally, so this review is based on reading the code.
| { | ||
| use LoggerSafeTrait; | ||
|
|
||
| private const TERM_ID_SUFFIXES = ['include_term_ids' => false, 'exclude_term_ids' => true]; |
There was a problem hiding this comment.
As far as I remember, Elementor Pro stores *_include_term_ids / *_exclude_term_ids as term_taxonomy_ids, not term_ids. Elementor_Post_Query::build_terms_query() resolves them with get_term_by('term_taxonomy_id', $id). Could you check this against the Elementor Pro source?
If that's right, this list is resolved wrongly downstream:
ElementAbstract::setRelations()(line 138) andContentRelationsDiscoveryService::normalizeReferences()both callgetTerm($id), which takes aterm_id.- Submissions are matched by
source_id, which is aterm_id.
On sites where the two IDs differ (older or multisite installs, terms that were split), we submit the wrong term, and on download we write a target term_id where Elementor expects a target term_taxonomy_id. That's the symptom described in WP-1020. Converting tt_id → term_id on the way in and term_id → tt_id when writing back should fix it.
| use LoggerSafeTrait; | ||
|
|
||
| private const TERM_ID_SUFFIXES = ['include_term_ids' => false, 'exclude_term_ids' => true]; | ||
| private const POST_ID_SUFFIXES = ['posts_ids' => false, 'exclude_ids' => true]; |
There was a problem hiding this comment.
These IDs are collected no matter which query mode the widget uses. Elementor keeps values of controls that are hidden, but only uses them when the mode matches:
posts_idsonly when{prefix}post_type === 'by_id'include_term_idsonly when{prefix}includecontainstermsexclude_ids/exclude_term_idsonly when{prefix}excludecontainsmanual_selection/terms
Collecting posts_ids is new in this PR. A widget that once had a manual selection and was then switched to "by category" will now submit all of those posts as related content, each with its own related content. Could we check the mode settings before adding the IDs?
| $return = []; | ||
| foreach ($flat as $item) { | ||
| assert($item instanceof Content); | ||
| if ($item->isRemapOnly()) { |
There was a problem hiding this comment.
Remap-only IDs are only replaced if the content is already translated when the translation is applied. If the excluded term or post is translated later, the page keeps the source ID until it's downloaded again. That's fine as a trade-off, but please document it in ELEMENTOR_DEVELOPMENT.md and the release notes so support knows what to expect.
2 participants
No description provided.