feat: P3.3 harvest item 9 — shared atomic convenience-param mapper - #35
Conversation
Port upstream 3.6.2's EMCP_Tools_Atomic_Widget_Map as
Elementor_MCP_Atomic_Widget_Map: the single source of the friendly-param →
typed-prop mapping for the 8 atomic widget types, consumed by BOTH the
add-atomic-* convenience tools and build-page — previously build-page
passed widget settings through raw, so an atomic widget given the same
friendly params (content, image_url, video_url, …) came out empty.
- The 8 convenience settings_fn closures now delegate to the map (their
inline mapping was the duplicated source build-page couldn't reach).
Registration bails class_exists-guarded per the fatal-proof convention.
- build-page routes atomic widget nodes through the map +
create_atomic_widget; the node's flat style params become a local class
exactly as the individual tools do (common + typography). Legacy
widgets keep the raw path.
- Correctness rides along, per the map's upstream verification:
- Atomic_Props::image() rebuilt to the upstream shape — image-src with
id XOR url (the old both-keys number/url shape is rejected with
image:invalid_value on 4.x, upstream #74 — the same XOR the fork's own
e-svg fix documents), id as image-attachment-id, alt INSIDE src, and
the attachment alt-meta write (the only alt Elementor renders for a
media-library image; the old top-level settings.alt was silently
discarded).
- New Atomic_Props::video_src(): on 4.x e-self-hosted-video's source is
a video-src SHAPE (id XOR url) — a bare url envelope is refused
outright on 4.2+ (upstream 3.6.2). The map keeps the fork's plain-url
shape on 3.x-experimental, and the fork's is_v4-aware content_prop /
svg envelope switches survive the extraction.
Test bootstrap: autoloader entry for the map, wp_get_attachment_url stub
(driven by $GLOBALS['_attachment_urls']), PHP 8.5 explicit-nullable fix in
the wp_check_filetype stub.
Board: item #9 (final planned item) in
references/docs/p3-3-atomic-harvest-scoping.md — #8 (change ledger) stays
blocked on an architecture decision; #10 deferred.
Tests: 18 new regression tests (827 total, 1940 assertions, green).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Retroactive post-merge review (the loop was interrupted by the Codex quota limit). This PR is already merged; any real finding will be fixed in a follow-up PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c366732a68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( '' !== $alt ) { | ||
| update_post_meta( $image_id, '_wp_attachment_image_alt', $alt ); |
There was a problem hiding this comment.
When add-atomic-image is called by a user who can edit the target page but cannot edit the supplied attachment, this unconditional update_post_meta() still changes that attachment because check_edit_permission() checks only the page. It also runs before the parent lookup and page save, so an invalid parent or failed save returns an error after the attachment alt has already been modified. Check permission for image_id and defer this side effect until the element is successfully persisted.
Useful? React with 👍 / 👎.
| $mapped = Elementor_MCP_Atomic_Widget_Map::settings( $widget_type, $settings ); | ||
|
|
||
| // Third arg: the raw node settings double as flat style params | ||
| // (build_common_props + build_typography_props read them). | ||
| return $this->factory->create_atomic_widget( $widget_type, (array) $mapped, $settings ); |
There was a problem hiding this comment.
When an existing build-page caller supplies the typed settings that atomic widgets require, such as title: {"$$type":"html-v3","value":"Hello"}, this now treats the envelope as a convenience value, sanitizes the array to an empty string, and discards every typed/custom prop the mapper does not recognize. The previous raw path preserved these settings, and the generic build-page schema still accepts them, so detect typed envelopes (or merge/pass them through) rather than unconditionally remapping every known atomic type.
Useful? React with 👍 / 👎.
| 'src' => array( | ||
| 'id' => self::number( $image_id ), | ||
| 'url' => self::url( $image_url ), | ||
| '$$type' => 'image-src', | ||
| 'value' => $src, | ||
| ), |
There was a problem hiding this comment.
Images created with this new shape are no longer discovered by Elementor_MCP_Content_Extractor::collect_image(): that method unwraps image.value and then reads id, url, and alt directly, while this change places them under image.value.src.value. Consequently the SEO/accessibility reports omit atomic images produced by these convenience and build-page paths; update the extractor to descend through src while retaining support for older shapes.
Useful? React with 👍 / 👎.
* fix: three P2 correctness gaps from the retroactive review round Follow-up to merged PRs #33 (coercion) and #34/#35 (style wiring, mapper). 1. Coerce items in ALREADY-WRAPPED typed arrays (was #33). The array branch in candidates_for() ran only when no $$type was present, so a caller who supplied the documented envelope with raw items — {'$$type':'attributes','value':[{'key':...,'value':...}]} — got no item coercion and Elementor still rejected the whole tree. A value carrying the MEMBER'S OWN envelope is now unwrapped to its payload, items coerced via get_item_type(), envelope rebuilt (mirroring the object-shape branch). A FOREIGN envelope is still left alone — that is the laundering rounds 3-5 closed. 2. Restrict root-key hoisting to atomic elements (was #34). On a classic or custom widget, / are ordinary control names — this repo's widget builder registers any sanitized control name — so hoisting them deleted the control's value from settings while reporting success, leaving the widget rendering its old value forever. Gated on a new structural is_atomic_element(): elType 'widget' with an e-* widgetType, or an e-* elType (atomic containers). Structural, not registry-based, so the write path never depends on a live Elementor. 3. Teach the content extractor the nested image-src shape (was #35). collect_image() unwrapped image.value and read id/url/alt directly, but the corrected e-image shape nests them under src.value — so every atomic image written by the convenience tools and build-page was invisible to the SEO and accessibility reports. It now descends through while still reading the older flat shape. 8 regression tests (835 total, green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: media library is authoritative for atomic attachment alt (Codex round-1 on #37) The image-src descent kept the payload alt for an attachment, so the later get_post_meta() fallback never ran. Elementor renders _wp_attachment_image_alt for an attachment and ignores src.alt (as Atomic_Props::image() documents), so a stale — or never-persisted, e.g. an alt write the user was not authorized to make — payload value could make the SEO and accessibility audits pass while the rendered image carried a different or empty alt. For the atomic image-src shape WITH an attachment id the library value now wins outright, and the payload alt is not a fallback: an empty library alt is the rendered state and the audit must see it. A url image (no attachment) still reports its payload alt, and classic image widgets are untouched — their settings-level alt is their own long-standing behaviour, not the atomic contract. 2 regression tests (837 total, green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: require a positive atomic signal, not the e-* slug prefix (Codex round-2) The gate treated the naming convention as authoritative, so a registered classic/custom widget with an e-*-prefixed slug and a 'styles' or 'editor_settings' control was still classified atomic — retaining exactly the data loss the gate was added to prevent. is_atomic_element() now requires a positive signal: 1. structural — the element already carries the atomic sibling-root keys (styles / editor_settings), or any of its settings is a typed $$type prop; both are atomic-only shapes and need no registry; 2. the registry — an atomic type declares get_props_schema(); classic widgets do not, so a non-empty prop schema is authoritative. When neither answers, the result is FALSE. Not hoisting merely leaves the write on a dead settings.styles key (the pre-hoisting behaviour), while hoisting wrongly destroys a real control value — the unsure case must fail toward preserving data. Test fixtures split into base_element() (a real atomic element, carrying the root keys the factory writes) and classic_element() (none of them), so each case is faithful rather than relying on the prefix. 3 regression tests: prefix-colliding custom widget stays classic; typed settings alone are a signal; a registered atomic schema is a signal (840 total, green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
P3.3 item #9 (final planned item) from
references/docs/p3-3-atomic-harvest-scoping.md:Elementor_MCP_Atomic_Widget_Map— single source of the friendly-param → typed-prop mapping for the 8 atomic widget types, consumed by both the add-atomic-* tools and build-page.Why
build-page passed widget settings through raw, so an atomic widget node with the same friendly params the convenience tools accept (
content,image_url/alt,video_url, …) came out empty — its complex props have no matching raw key. The mapping lived duplicated inside 8 tool closures where build-page couldn't reach it.How
settings_fnclosures delegate to the map; registration bailsclass_exists-guarded (fatal-proof convention).create_atomic_widget; flat style params on the node become a local class (common + typography), exactly like the individual tools. Legacy widgets keep the raw path.Atomic_Props::image()rebuilt: image-src with id XOR url (old both-keys shape →image: invalid_valueon 4.x, upstream #74 — the same XOR the fork's own e-svg fix documents), id asimage-attachment-id, alt INSIDE src +_wp_attachment_image_altwrite (the only alt Elementor renders for attachments; the old top-levelsettings.altwas silently discarded).Atomic_Props::video_src(): on 4.xe-self-hosted-video'ssourceis a video-src SHAPE — bare url envelope refused outright on 4.2+ (upstream 3.6.2).Tests
18 new regression tests (
P33AtomicWidgetMapTest): registry, per-type mapping incl. defaults and is_v4 branches, image XOR + alt-meta, svg single-url, video-src both paths, build-page routing (typed settings, style params → local class + classes ref, legacy fallback). Full suite: 827 tests, 1940 assertions, green.Note: Codex + Copilot both at quota — review loop runs retroactively when quota resets, merging per owner's process decision (as with #33/#34), after human review.
🤖 Generated with Claude Code