add Elementor 4 support (WP-1000)

[vsolovei-smartling]

No description provided.

[PavelLoparev]

Review summary

Mostly a clean refactor + v4 surface area, but one 🔴 correctness regression in ElementAbstract::setRelations that I think will fail an existing test (see inline comment). After that's reverted, the main thing worth a second look is the service-ID rename in services.yml.

On the service-ID rename (content.elementor → content.elementor3)

The class rename (ExternalContentElementor → ExternalContentElementorAbstract + a 16-line ExternalContentElementor3 shell) is the right call — standard "extract abstract base, keep concrete name for the original behavior" pattern, and the alternative (using the concrete class as both the v3 handler and the parent of v4) would muddle responsibilities.

The service ID rename in inc/config/services.yml is the part I'd push back on:

• Service IDs are part of the plugin's DI contract. Anything that resolves @content.elementor or @elementor.factory by string — other Smartling plugins, customer customizations, e2e tests, hook callbacks reading from the container — silently breaks with ServiceNotFoundException.
• The benefit is purely cosmetic symmetry with content.elementor4.
• The class name and the service ID don't have to match. DI is exactly the layer where they decouple.

Suggested: keep content.elementor / elementor.factory pointing at the new *3 classes, and only add the content.elementor4 / elementor.factory4 siblings. If you want the suffix in code too, use a deprecated alias rather than a hard rename:

content.elementor3:
  class: Smartling\ContentTypes\ExternalContentElementor3
  arguments: [...]

content.elementor:
  alias: content.elementor3
  deprecated: 'Use content.elementor3'

Cross-cutting

1. ElementFactory4 extends ElementFactory3 — shared mutable state via inheritance; both register into the same $this->elements map keyed by getType(). A future v4 element reusing a v3 type name would silently overwrite. Prefer composition: v4 holds a v3 factory and consults it as a fallback. (No collision today.)
2. testMixedElementorVersionsInSamePage — the name implies a real scenario, but in production both handlers are version-gated by getSupportLevel(), so they can't both process one submission. Misleading test name.
3. No rollback path if v4 causes an incident — only fix is a hotfix removing content.elementor4 from services.yml. Consider a feature flag (constant or option) so customers can opt out without a code deploy.
4. Coverage gap in Elements4/*: extractTypedValue handles only string and html-v3. Any other $$type Elementor 4 might use silently drops out of translation. Worth confirming against Elementor v4's settings schema.
5. Stale docs — docs/ELEMENTOR_DEVELOPMENT.md:171,279 still reference the removed ExternalContentElementor class.
6. composer.json — clue/graph-composer: "*" has no usage in the repo. Either remove it or pin + document.

Assessment

Ready to merge? No — with fixes.

Reasoning: The inverted is_string($contentType) check in ElementAbstract::setRelations is a regression affecting every v3 widget with related content (Image, Gallery, Posts, MegaMenu, NestedAccordion, etc.) and breaks an existing test. After that's reverted, reconsider the service-ID rename (or add a deprecated alias) and clean up composer.json. Everything else is polish.

---

🤖 AI-assisted review posted via /sl-core-dev:claude-pr-review-local (Claude Opus 4.7). Findings verified by hand against the diff; please push back on anything that misreads context.

[PavelLoparev]

🔴 critical — inverted check, likely a copy-paste/refactor mistake

-        if ($contentType === false) {
+        if (is_string($contentType)) {
             $contentType = ContentTypeHelper::CONTENT_TYPE_UNKNOWN;
         }

The original guard said "if we couldn't determine the content type, default to UNKNOWN." The new code does the opposite: whenever a valid content type is returned, it gets overwritten with CONTENT_TYPE_UNKNOWN. Downstream, getTargetId() then omits FIELD_CONTENT_TYPE from the submission lookup, so relation mapping can match the wrong submission across content types. This breaks every v3 widget with related content (Image, Gallery, Posts, MegaMenu, NestedAccordion, etc.) and EImage in v4. ExternalContentElementor3Test::testSetContentFieldsRelationsChange should fail in CI on this.

Revert:

if ($contentType === false) {
    $contentType = ContentTypeHelper::CONTENT_TYPE_UNKNOWN;
}

Or, if the intent was to widen the guard for non-string returns (e.g. null, array):

if (!is_string($contentType) || $contentType === '') {
    $contentType = ContentTypeHelper::CONTENT_TYPE_UNKNOWN;
}

[vsolovei-smartling]

Fixed

[PavelLoparev]

🟡 clue/graph-composer: "*" has no reference in the repo (no script, no doc, no Makefile). Unversioned * in require-dev is a supply-chain risk even for the dev environment. Either remove or pin to a specific version with a comment on what it's used for.

[vsolovei-smartling]

Removed

[PavelLoparev]

🟡 ElementFactory4 extends ElementFactory3 shares mutable state — loadElements() mutates $this->elements keyed by getType() from both directories. If a future v4 element reuses a v3 type name, the later registration silently wins. No collision today, but consider composition (v4 holds a v3 factory and falls back to it) so the two registries stay independent.

[vsolovei-smartling]

No issue, future elements should supersede previous ones

[PavelLoparev]

🟡 ElementAbstract::setTargetContent includes assert($content instanceof Content); before the same call; the v4 path doesn't. Minor consistency gap — if getOwnRelatedContent() is ever extended to return non-Content items, this is a defense-in-depth gap.

[vsolovei-smartling]

Removed assertion

[PavelLoparev]

🔵 ExternalContentElementor3 and ExternalContentElementor4 are both 16-line shells whose only content is getMinVersion()/getMaxVersion(). Consider making the version range data-driven (constructor arg, or a protected property override on the abstract) so a future v5 needs only a services.yml entry, not a new file.

[vsolovei-smartling]

Consistent with other external content, won't change

[PavelLoparev]

🟣 IMAGE_ID_PATH = 'image/value/src/value/id/value' — what about the URL (image/value/src/value/url/value) and alt text? In v3 the connector also translates image.alt. Are these intentionally ignored, or a follow-up?

[vsolovei-smartling]

Looks like there's no support for them yet in the atomic editor, and I can't guess what keys they will use

[PavelLoparev]

🔵 Only placeholder is extracted here (same for EFormTextarea). Elementor's form widget typically also stores help text and validation messages. Worth checking the v4 form widget settings schema and adding the remaining translatable keys.

[vsolovei-smartling]

Once again, these will probably appear somewhere in settings at some point of time, but I can't predict where

[PavelLoparev]

🟣 Tests construct the handler directly and bypass getSupportLevel()/getMinVersion()/getMaxVersion(), so they don't validate that the v4 handler is actually selected for Elementor 4.x and rejected for 3.x. Consider a testCanHandle-style test asserting v3 returns SUPPORTED for Version=3.21.0 and NOT_SUPPORTED for 4.0.0, and vice-versa for the v4 handler.

[vsolovei-smartling]

Done