Repository navigation
[Improvement]: [Seo] Allow redirect targets to be data objects or assets - #2155
dolmit-tanel-paaro wants to merge 9 commits into
Conversation
Accepts and returns the targetType introduced in pimcore/pimcore#18293, so a redirect target can be a document, an asset or a data object instead of always being resolved as a document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Typing an asset or data object path previously resolved only against documents, so it was stored as a plain URL. The submitted type now restricts the lookup for disambiguation, and is detected when absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The public response constructor breaks compatibility, and unsupported object targets can be saved as nonfunctional redirects.
2 open issues
What changed in this PR
Verdict: Needs changes. This PR extends Studio’s SEO redirect API to support asset and data-object targets alongside documents, using the companion core changes in pimcore/pimcore#18293.
Changes:
- Adds
targetTypeto redirect response and request schemas. - Resolves submitted paths by explicit type or automatic detection, while retaining legacy document fallback during hydration.
- Adds focused hydrator tests for documents, assets, legacy targets and plain URLs.
The service and hydrator changes address the document-only assumption at the appropriate boundary (RedirectsService.php:65,94,237–255; RedirectHydrator.php:60–70). Both add and update use the shared resolution logic. Tests cover hydration, but service eligibility behavior and full integration were not verified by execution; request documentation also needs alignment with automatic detection.
| File | Description |
|---|---|
tests/Unit/Bundle/Seo/Hydrator/RedirectHydratorTest.php |
Tests typed, legacy and plain-URL hydration. |
src/Bundle/Seo/Service/RedirectsService.php |
Resolves paths and persists target types. |
src/Bundle/Seo/Schema/RedirectUpdateParameters.php |
Accepts an optional target type on updates. |
src/Bundle/Seo/Schema/RedirectAddParameters.php |
Accepts an optional target type on creation. |
src/Bundle/Seo/Schema/Redirect.php |
Exposes target type in responses. |
src/Bundle/Seo/Hydrator/RedirectHydrator.php |
Resolves numeric targets using their element type. |
Review effort: Balanced
| example: 'document', | ||
| nullable: true | ||
| )] | ||
| public readonly ?string $targetType, |
There was a problem hiding this comment.
Fixed in 60781d7. targetType is now the last constructor argument with a null default, so positional arguments keep their positions and a named call that omits it still works. It stays in the schema's required list, since the response always carries it (null for a redirect to a plain URL).
| if ($element !== null) { | ||
| $redirect->setTarget((string)$element->getId()); | ||
| $redirect->setTargetType($candidate); |
There was a problem hiding this comment.
Agreed, and this is already addressed - the change landed in pimcore/pimcore@683c49e759 and 86ffed8 before this review ran, so the diff you looked at predates it.
TargetResolverInterface::isUsableAsTarget() decides whether an element can be a redirect target at all - not a folder, not a snippet or an email, and not a data object whose class has no link generator - and resolveElement() returns null for anything it rejects. That gives exactly the three behaviours you describe:
- eligibility is checked before the path is converted;
detectElement()passes over an ineligible candidate, so an auto-detected target that matches one stays a literal URL with no type;- an explicitly submitted target type that resolves to nothing usable is rejected with
No <type> usable as a redirect target found at "<path>"(422), which is also what the CSV import does for the same row.
RedirectHandler::resolveElementTarget() asks the same method rather than restating the rule, and still re-checks at request time - a class can lose its link generator after the redirect was saved.
Keeps the path<->element-ID rules in one place, so the Studio API and the CSV import cannot drift apart on what a redirect target means. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Detection is now an explicit call: a client that sends a target type restricts the lookup to that tree, one that sends none has the type detected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- an update that touches neither the target nor its type no longer re-resolves it, so a round trip of a redirect whose element is gone keeps the type - a submitted target type that names no such element is rejected, as the CSV import already does, instead of silently storing a plain URL - targetType is nullable, so null belongs in its OpenAPI enum - the target type is validated before the loaded model is mutated Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A required argument in the middle of the constructor shifts every positional argument after it and breaks named calls that do not pass it. The field stays required in the response schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Changes in this pull request
Resolves pimcore/platform-version#707
Tracked in pimcore/pimcore#15398
Companion to pimcore/pimcore#18293, which lets a redirect target be a data object or an asset instead of always a document. Part of pimcore/pimcore#15398.
Without this, the Studio API resolves every redirect target as a document, so the new target types are only reachable via SQL or CSV import.
BundleSeoRedirect,BundleSeoRedirectAddandBundleSeoRedirectUpdatecarry atargetType(document/asset/object).RedirectHydratorresolves a numeric target back to its path using that type, falling back todocumentso redirects created before target types existed keep working.RedirectsServiceresolves the submitted path to an element ID:targetTyperestricts the lookup, which is how a path existing in more than one tree is disambiguated;InvalidArgumentException.RedirectHydratorTestcovers document, asset, legacy untyped and non-element targets.Additional info
Requires the core PR:
targetTypeand thePimcore\Model\Element\ElementTypeenum come from pimcore/pimcore#18293.targetTypeis optional in the add/update schemas, so clients that do not send it behave exactly as before.The UI counterpart is pimcore/studio-ui-bundle#4269.
Verified against a 2026.x instance with Studio: adding/updating with each target type, type detection, an explicit type restricting the lookup, legacy untyped rows, and the generated OpenAPI schema.