Fix wpseo_titles being overwritten with translated values on frontend (WPML) - #23187
Fix wpseo_titles being overwritten with translated values on frontend (WPML)#23187enricobattocchi wants to merge 8 commits into
Conversation
enricobattocchi
added
the
changelog: bugfix
Coverage Report for CI Build 0Coverage decreased (-0.4%) to 55.051%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
Fixes a WPML/WPML String Translation interaction where frontend-triggered writes to the wpseo_titles option could persist translated strings as the primary-language originals by eliminating frontend writes and ensuring logo meta is populated at save time.
Changes:
- Add a
Logo_Meta_Watcherintegration that populatescompany_logo_meta/person_logo_metaonpre_update_option_wpseo_titles. - Stop
Image_Helper::get_attachment_meta_from_settings()from persisting computed logo meta back into options (compute-only fallback remains). - Add unit + WP integration tests covering the watcher behavior and the updated image-helper behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/integrations/watchers/logo-meta-watcher.php |
New watcher that keeps logo meta consistent at option-save time to avoid unsafe frontend resaves under WPML. |
src/helpers/image-helper.php |
Removes the write-back side effect from get_attachment_meta_from_settings, preventing frontend from updating wpseo_titles. |
tests/WP/Admin/Watchers/Logo_Meta_Watcher_Test.php |
WP integration tests to validate the option pipeline populates logo meta on save. |
tests/Unit/Integrations/Watchers/Logo_Meta_Watcher_Test.php |
Unit tests covering watcher logic branches (preserve supplied meta, recompute, clear, etc.). |
tests/Unit/Helpers/Image_Helper_Test.php |
Unit tests verifying cached-read and compute-on-miss behavior without writing to options. |
composer.json |
Updates CS threshold to reflect current baseline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leonidasmi
left a comment
leonidasmi
left a comment
There was a problem hiding this comment.
CR: Some (minor?) comments below, plus one more consideration about an addition in tests:
- If we ever add more
wpseo_titlessettings inSettings_Integration::DISALLOWED_SETTINGS, then we'll have resurfaced the issue we fixed with this PR, unless we dont set those settings in the frontend. Maybe we can add a test with a relevant comment, that checks theSettings_Integration::DISALLOWED_SETTINGS, so that if we ever add to that const, we do that having the above in mind
| if ( $image_meta ) { | ||
| $this->options_helper->set( $setting . '_meta', $image_meta ); | ||
| } |
There was a problem hiding this comment.
I would like us to second-guess the decision to remove this.
I understand why we do it, I'm just wondering whether we should. I think this comes into play in the event of our solution with the watcher is not working properly (as this would be the only case where we get into the if ( ! $image_meta ) { branch).
If there ever is a case where $image_meta is not present while $image_id is, then I would probably rather have the translation bug present than keep calculating the image meta on the frontend, because storing it persistently is removed.
But you might disagree, so let's talk about it :)
There was a problem hiding this comment.
I see your point but I expect sites to be in the ! $image_meta state only if they have
- changed the logo
- not visited the frontend
- then upgraded
- not saved the settings anymore
very small chance of it happening, recalculation is not cheap but people have a way to prevent it for good (resaving the settings).
I prefer this than having an option read triggering an option write in the frontend, which is definitely more deceitful...
| * @return array<string, string|int|bool|array<string, string|int|bool>>|false The — possibly repopulated — value to store. | ||
| */ | ||
| public function ensure_logo_meta( $new_value ) { | ||
| if ( ! \is_array( $new_value ) ) { |
There was a problem hiding this comment.
I would like us to add a technical choice entry that describes why we didn't add a check of whether the logo changed or not. I think this describes the reasons we want the meta to be re-generated regardless, but I think it's worth a mention since we're (slightly) decreasing the performance of every saving of wpseo_titles.
| $new_value[ $meta_key ] = ( \is_array( $computed ) && $computed !== [] ) ? $computed : false; | ||
| } | ||
|
|
||
| return $new_value; |
There was a problem hiding this comment.
WPSEO_Options::save_option returns $saved_option[$key] === $options[$key]. If anyone calls options_helper->set( 'company_logo_meta', false ) while a logo id is configured, the watcher overwrites the value with a computed array, the equality check fails, and set() returns false, even though the save succeeded.
Today's callers (AIOSEO importer, First-Time Config) don't check that return value, so nothing breaks but future callers might do and they would get a false result.
Maybe mostly theoretical issue, but it's another thing that adding a check of whether the logo changed or not solves (aside from the performance issue I shared above)
92f9972 to
51fdb69
Compare
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
Add a Logo_Meta_Watcher hooked on pre_update_option_wpseo_titles that derives company_logo_meta and person_logo_meta from their matching attachment ids and injects them into the value being written, so the stored wpseo_titles option stays self-consistent on every save. The Yoast settings UI deliberately strips these two keys (they live in Settings_Integration::DISALLOWED_SETTINGS because they hold compiled image-variation data the UI should not round-trip), which previously left the cache empty until the next frontend schema render rebuilt it in-place. Moving the derivation to the save path is the behaviour the original 2021 perf-cache commit assumed was already happening and removes the need for any frontend code to write wpseo_titles itself. Covered by a Mockery-based unit test plus WP integration tests that exercise the full update_option pipeline — populate on first save, recompute on id change, clear on id removal, both for company_logo and person_logo. Refs #22549.
Image_Helper::get_attachment_meta_from_settings() used to populate its
own cache by calling options_helper->set( 'company_logo_meta', ... )
when the stored meta was empty. That write went through
WPSEO_Options::save_option(), which does get_option('wpseo_titles') ->
modify -> update_option('wpseo_titles', ...). On sites running a
translation plugin that registers wpseo_titles as an admin-text (such
as WPML with String Translation), the get_option read returns the
translated bundle on the frontend, and the subsequent update_option
persists those translations as the primary-language values, silently
corrupting every translated title, metadesc and breadcrumb string.
Logo_Meta_Watcher now keeps the cache populated from the save path, so
the helper only needs to read. The in-method compute fallback stays in
place as a safety net for the narrow post-deploy window on sites that
were last saved but not yet re-visited on the frontend; it no longer
writes back.
Also tightens a few pre-existing @return annotations to satisfy the
Yoast Coding Standard on the now-touched file.
Fixes #22549.
* Scope the WP integration test teardown to the watcher's own filter
callback so unrelated listeners on pre_update_option_wpseo_titles
are not incidentally removed.
* Reword a docblock sentence on get_attachment_meta_from_settings for
clarity ("On the narrow window" → "In the narrow window").
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Locks the contents of `Settings_Integration::DISALLOWED_SETTINGS['wpseo_titles']` so any future addition to that subarray triggers a deliberate review of the frontend-write risk that #22549 fixed. The docblock points at `Logo_Meta_Watcher` as the canonical pattern for repopulating stripped keys at save time.
`WPSEO_Options::save_option` performs a read-modify-write on the option, so a caller that updates only `<prefix>_id` arrives at `pre_update_option_wpseo_titles` with the previous attachment's `<prefix>_meta` blob still attached. The watcher's earlier "caller supplied meta — respect it" branch treated that carried blob as authoritative and let the stale variation survive across an id change. Pass `$old_value` to `Logo_Meta_Watcher::ensure_logo_meta` and only respect a supplied meta blob when the id is unchanged. The `set( '_meta', false )` calls in the First-Time Configuration action and the AIOSEO importer become no-ops as a result — keep them for now but flag them as vestigial so a future cleanup pass can remove them. Adds unit and WP integration coverage for the "id-changed-with-carried-meta" branch.
Move the long rationale below the one-line summary so the docblock follows the standard "short description + blank line + extended description" PHPDoc shape.
51fdb69 to
ac48505
Compare
Context
Issue #22549 reports that on sites running Yoast SEO alongside WPML + WPML String Translation, the whole
wpseo_titlesoption gets silently overwritten with the secondary-language translations after saving the Yoast Settings and then opening a frontend page in a secondary language. Once corrupted, the Yoast Settings UI and the primary-language frontend both start showing the translated strings as if they were the originals.The chain of events on
trunk:company_logo_metaandperson_logo_metafrom its payload (they live inSettings_Integration::DISALLOWED_SETTINGS), so every admin save wipes these keys to their defaults.Image_Helper::get_attachment_meta_from_settings()sees the missing_meta, recomputes it from the attachment id, and writes it back viaoptions_helper->set('company_logo_meta', …).WPSEO_Options::save_option(), which doesget_option('wpseo_titles')→ modify →update_option('wpseo_titles', …). On the frontend,get_option('wpseo_titles')is filtered by WPML'sicl_st_translate_admin_string, returning the translated bundle. The subsequentupdate_optionpersists those translations as the primary-language values.Summary
This PR can be summarized in the following changelog entry:
wpseo_titlescould be silently replaced by their WPML translations on multilingual sites.Relevant technical choices
Logo_Meta_Watcherintegration hooks thepre_update_option_wpseo_titlesfilter and populatescompany_logo_meta/person_logo_metafrom the matching_idat save time, keeping the stored option self-consistent and matching the behaviour the 2021 perf-cache commit apparently assumed was already in place.Image_Helper::get_attachment_meta_from_settings()no longer writes the meta back to the option. It keeps a compute-on-miss fallback so schema output stays correct during the narrow window on sites that were last saved but not yet re-visited on the frontend at the moment of the upgrade; it never persists the result, which means no frontend code path re-saveswpseo_titlesanymore.DISALLOWED_SETTINGSstripping stays as-is (the UI still shouldn't round-trip these compiled blobs), and the titles-option validator keeps its default-to-falsebehaviour since the pre-update filter repopulates_metabefore the DB write lands._meta. Sites with any frontend traffic already have_metapopulated by the old write-back, and the helper's compute fallback covers the narrow "saved but not yet visited" case until the next admin save triggersLogo_Meta_Watcher.Test instructions
Test instructions for the acceptance test before the PR gets merged
Reproducing the original bug requires WPML + WPML String Translation configured for at least two languages.
[wpseo_titles]title-404-wpseo(filter on theadmin_texts_wpseo_titlesdomain) and add a French translation — e.g.Page non trouvée %%sep%% %%sitename%%./fr/sample-page/.<title>tag.Expected on this PR: the English 404 title is still the English one you configured. In Yoast → Settings the primary-language fields still show the original text; in WPML → String Translation the
wpseo_titlesoriginals remain in the primary language.Expected on
trunktoday (bug): the English 404 title renders the French translation; Yoast settings start displaying the French strings as the primary-language values.Note: Reset indexables every time you want to check the 404 pages' title, because this is cached in the relevant indexable. Also reset options.
Additional scenarios to spot-check:
wpseo_titles.company_logo_metainwp_optionsshould immediately match the new attachment (differentwidth/height/id), without needing a frontend visit.wpseo_titles.company_logo_metashould befalse.Relevant test scenarios
Test instructions for QA when the code is in the RC
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
wpseo_titlesoption (all Yoast Settings screens touching that option, plus programmatic callers likeWPSEO_Options::set).ImageObjectnode.company_logo_meta/person_logo_metavia the options helper. The watcher respects a caller-supplied meta blob so the importer's behaviour is unchanged.Other environments
Documentation
Logo_Meta_Watcherplus the updated docblock onImage_Helper::get_attachment_meta_from_settings()explain the WPML interaction and reference this issue.Quality assurance
grunt build:imagesand commited the results, if my PR introduces new images or SVGs.Innovation
Fixes #22549