Skip to content

Make identifier resolution resource aware in the Doctrine and Eloquent links handlers #8494

Description

@soyuka

Follow-up to #8491.

#8491 lets a uri variable's parameter provider transform the value used to query the resource, and lets ReadLinkParameterProvider opt into writing the resolved resource into the uri variables (constructor flag, or the write_uri_variable extra property per link). Working on the resource rather than the identifier is usually easier when writing a custom provider.

That opt-in currently only works for resources whose provider does not query a persistence layer. On a Doctrine-backed link it fails with:

Object of class App\Entity\Company could not be converted to string

because the links handler compares the identifier column and binds the value with an explicit Doctrine type:

// src/Doctrine/Orm/State/LinksHandlerTrait.php
$queryBuilder->andWhere("$joinAlias.$identifierProperty = :$placeholder");
$queryBuilder->setParameter(
    $placeholder,
    $this->getIdentifierValue($identifiers, $hasCompositeIdentifiers ? $identifierProperty : null),
    $fromClassMetadata->getTypeOfField($identifierProperty)
);

Proposal

Make identifier resolution accept a resolved resource and read the link's identifier off it, keeping the query shape unchanged. Reading the link's identifier (e.g. a name slug) rather than assuming the Doctrine primary key avoids the identifier-divergence problem.

Both backends funnel through a single method, so this is two small changes rather than three parallel ones:

Backend Method Call sites covered
Doctrine getIdentifierValue() in src/Doctrine/Common/State/LinksHandlerTrait.php ORM (4), ODM (2), PersistProcessor (4) — all share it
Eloquent buildQuery() in src/Laravel/Eloquent/State/LinksHandler.php all 5 entry points funnel through it

symfony/property-access is already a dependency.

Wrinkle

getIdentifierValue(array &$identifiers, ?string $name = null) only receives the property name when the link has composite identifiers; otherwise it is null and the method does array_shift($identifiers). Reading a property off an object requires knowing which property, and only the caller has it — so $identifierProperty needs to be passed unconditionally at the 6 Doctrine call sites. It is a private method on an @internal trait, so there is no BC surface.

Risk

PersistProcessor shares getIdentifierValue() and uses it to build references on write. It needs verifying that a resolved resource cannot reach it, or an explicit guard.

Suggested sequence

  1. Pass $identifierProperty unconditionally at the Doctrine call sites.
  2. Make getIdentifierValue() resource aware.
  3. Audit PersistProcessor.
  4. Gate: LinkProviderParameterTest::testLinkSecurityWithSlug green with write_uri_variable enabled on a Doctrine-backed link.
  5. Eloquent buildQuery().
  6. ODM.

The bulk of the effort is verification across the three backends (the Laravel suite runs under testbench, ODM needs the mongodb environment), not the code itself.

Beyond this

Once identifier resolution is resource aware, flipping the default so ReadLinkParameterProvider always writes the resource becomes a separate, explicit BC decision. It is a real break for consumers independent of Doctrine: plain-ApiResource providers that read uri variables assume scalars — the Issue7939BazResource fixture does (string) ($uriVariables['barId'] ?? '') — so it would need an upgrade note. PreservesUriVariableInterface stays meaningful either way, since consumers still need resources kept out of the uri variables.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions