fix: disable smilie conversion for imported static content (#780) - #788
Conversation
chubes4
left a comment
There was a problem hiding this comment.
Thanks for taking this on. The explicit import option and receipt evidence are the right direction, but the companion-plugin implementation does not reliably apply the policy.
The blocking issue is that ArtifactCompiler emits companion_plugin_payload only when custom blocks or runtime islands require one. Static_Site_Importer_Theme_Generator therefore copies disable_smilies into a plugin payload only when an unrelated companion payload already exists. A simple static site containing literal :) can have no companion payload, produce no plugin, and still return a receipt claiming runtime_policy.disable_smilies: true.
The generated callback also removes only the the_content filter. Core gates every convert_smilies() call on the use_smilies option, including direct template/template-part calls and the excerpt, caption, comment, and widget filters. The site option is the existing owning WordPress primitive and avoids making this policy depend on generated-plugin intent.
Could you revise this around the existing site-plan activation path?
- Keep
disable_smiliesin the normalized import input and materialization ability contract. - Remove the companion-plugin payload/scaffold changes for this policy.
- When materialization activates the imported theme and the policy is enabled, call
update_option( 'use_smilies', false ). A non-activating import should not mutate the currently active site's global option. - Make receipt evidence distinguish requested policy from applied policy. It should not report the policy as applied when
activateis false or when no runtime mutation occurred. - Cover a plain artifact that emits no companion payload, an activating import that leaves
convert_smilies( 'Hello :)' )unchanged, a non-activating import that leaves the existing option untouched, and the default-policy decision.
One product decision also needs to be explicit: issue #780 says imported literal smilies remain text, but this PR defaults the option to false, so ordinary imports retain the bug. If this remains opt-in, callers must wire it deliberately and the issue's default acceptance is not complete.
The branch currently conflicts with main and has no CI results, so it will also need a refresh after the architectural change.
AI assistance: OpenAI GPT-5.6 Sol via OpenCode reviewed the current SSI, Blocks Engine, and WordPress core paths and helped draft these findings. Chris Huber is responsible for the review.
… option
Imported literal emoticon text (:) was lost to WP core smilie conversion.
Set the owning site option use_smilies=false when materialization activates
the imported theme, instead of a companion-plugin filter that is never
scaffolded for plain artifacts. The receipt reports
runtime_policy.disable_smilies as {requested, applied}; applied is only
true when the activation-time update_option actually ran. Defaults on so
ordinary imports keep literal text.
Refs Automattic#780 Automattic#788
68dda7e to
48ab1a3
Compare
faisalahammad
left a comment
There was a problem hiding this comment.
Addressed in 48ab1a3. Revised around the site-plan activation path as requested:
- Option kept in contract —
disable_smiliesstays in the normalized import input (website-artifact-import-inputschema, default, bool coercion) and thematerialize-wordpress-site-planability schema. - Companion-plugin approach removed — no
disable_smiliesin companion payload/scaffold or theme-generator codegen. The policy no longer depends onArtifactCompileremitting a payload. - Owning primitive — on an activating import with the policy enabled, materialization calls
update_option( 'use_smilies', false )inside the existing activation gate (while activatingblock inmaterialize_prepared()). A non-activating import never touches the active site's global option. - Requested vs applied receipt evidence —
completed.runtime_policy.disable_smiliesis now{ "requested": bool, "applied": bool }.appliedis only ever set in the same branch as theupdate_optionwrite (so it is true only when a runtime mutation actually ran), and the write itself is fail-closed (disable_smilies_not_appliedreceipt onupdate_optionreturningfalse). - Smokes —
smoke-wordpress-site-plan-materializer.phpcovers: non-activating default (requested true / applied false / option untouched), non-activating with explicit false, activating default (option flipped,convert_smilies('Hello :)')returns literal text, requested+applied true), activating with explicit false. The plain-artifact case (no companion payload) is exercised because the materializer smoke never scaffolds a companion plugin.
Product decision is now explicit and defaulted per issue #780: the policy defaults to true, so ordinary activating imports disable smilie conversion and literal :) stays text. Opt-out is --no-disable-smilies (CLI) or disable_smilies: false (ability/REST).
Branch was reset onto current main and refreshed (the stale 66-behind history was dropped entirely — the old approach is gone, not reverted-in-place).
Tests: npm test 42 passed / 0 failed, npm run test:inventory clean, targeted smokes green.
|
The revised site-option direction has one blocking idempotency defect: WordPress Please treat an existing I independently replayed the four-surface #780 fixture against current AI assistance: OpenAI gpt-5.6-sol via OpenCode inspected WordPress option semantics, ran the four-surface replay, and helped draft this review. Chris Huber is responsible for the findings. |
|
Addressed. The idempotency defect is fixed and covered:
Branch was reset onto current Tests: One follow-up for a future pass (pre-existing, not introduced here, not blocking): on a fresh site where the |
… option
Imported literal emoticon text (:) was lost to WP core smilie conversion.
Set the owning site option use_smilies=false when materialization activates
the imported theme, instead of a companion-plugin filter that is never
scaffolded for plain artifacts. The receipt reports
runtime_policy.disable_smilies as {requested, applied}; applied is only
true when the activation-time update_option actually ran. Defaults on so
ordinary imports keep literal text.
Refs Automattic#780 Automattic#788
fb25e52 to
9bd21ca
Compare
Summary
Imported static sites lose literal text because WordPress core maps emoticons like
:)to an external emoji image (wp-includes/images/smilies). That shifts heading geometry, breaks offline visual capture, and makes the imported page no longer match the source.This adds a
disable_smiliesboolean option (defaulttrue) to the import and site-plan materialization contracts. When materialization activates the imported theme and the option is enabled, the plugin sets the coreuse_smiliesoption tofalse, so imported literal:)stays text and no external emoji image is requested. The materialization receipt records the policy undercompleted.runtime_policy.disable_smiliesas a{ requested, applied }pair, so the requested policy is explicit and the applied policy is only reported when the runtime mutation actually ran.Fixes #780
Changes
includes/class-static-site-importer-website-artifact-import-input.php
Declares
disable_smiliesinSCHEMA_PROPERTIES, defaults it totrueinnormalize(), and adds it to the bool-cast list. The default is part of the normalized input contract, so CLI, REST, and ability entrypoints that build import input get it throughnormalize().includes/abilities.php
Adds
disable_smilies(boolean) to thematerialize-wordpress-site-planability input schema. Theimport-website-artifactability inherits it fromSCHEMA_PROPERTIES.includes/class-static-site-importer-wordpress-site-plan-materializer.php
When the
activatearg is set anddisable_smiliesis not explicitlyfalse, setsupdate_option( 'use_smilies', false ). The call is guarded by the activate block, so a non-activating import never mutates the currently active site's global option. If the write fails, materialization fails closed (disable_smilies_not_applied) instead of claiming the policy was applied.The receipt records the policy under
completed.runtime_policy.disable_smilies:requestedis the caller's flag, defaulting totrue.appliedis onlytruewhen the activation-timeupdate_option( 'use_smilies', false )actually ran.Why the site option instead of a companion-plugin filter
The previous approach emitted a
remove_filter( 'the_content', 'convert_smilies', 20 )call from the generated companion plugin. That only touches thethe_contentfilter, and the companion plugin is only scaffolded when a site has custom blocks or preserved JS. A plain static site with literal:)can emit no companion payload, produce no plugin, and still return a receipt claiming the policy was applied. Core gates everyconvert_smilies()call on theuse_smiliesoption, so the site option is the owning primitive: setting it tofalseis honored everywhere the conversion would otherwise run, and the receipt only reportsappliedwhen the mutation actually happened.CLI
--no-disable-smiliesis accepted byimport-website-artifact,import-url, andmaterialize-wordpress-site-planto opt out of the default-on policy.Tests
smoke-wordpress-site-plan-materializer.php: activating import setsuse_smiliestofalseand leavesconvert_smilies( 'Hello :)' )unchanged; non-activating import leaves the option untouched; receipt reports{ requested, applied }for default-on, explicit opt-out, and activating cases.smoke-website-artifact-import-input.php:disable_smiliesdefaults totrue, and'1'/'0'coerce to bool.Testing
Test 1: Default (flag on)
:)(for example<p>Hello :)</p>) with an activating import (wp static-site-importer import-url https://example.com --activate).Hello :)as text, with no<img src=".../wp-includes/images/smilies/...">markup in the rendered HTML.Test 2: Opt out
--no-disable-smilies.:)renders as the external emoji image, core default behavior unchanged.Automated: all smokes pass under
npm test(42 passed, 0 failed; wordpress-runtime tests require a live WP and are skipped in the fast lane).npm run test:inventoryis clean.