Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework to improve and simplify identifiers management #3825

Merged
merged 4 commits into from Nov 30, 2020

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Nov 10, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tickets adaptation of #3664 with proper depreciation also fixes api-platform/api-platform#1518 and others
License MIT
Doc PR TBD

Move logic of identifiers extractor upwards, use only identifier converter downwards

TODO:

  • fix tests
  • open another PR that deprecates plain identifiers in favor of a documented normalizer
  • Document the work to be done on 3.0

When removing deprecations on 3.0 we should:

  • rename IdentifierDenormalizer to IdentifierConverter
  • remove the use of the symfony DenormalizerInterface for Identifier/Normalizers as their job is slightly different from normalizers in which they just convert the type from string to integer, boolean etc.
  • remove IdentifiersExtractor from IriConverter and every data provider that may use it.
  • remove IdentifierConverterInterface::HAS_IDENTIFIER_CONVERTER

The diff at https://github.com/api-platform/core/pull/3664/files can be used as a reference.

@soyuka soyuka marked this pull request as ready for review November 18, 2020 07:46
}

return $id;
$identifiersKeys = $subresourceIdentifiersKeys;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change with the rework of the subresources

src/Bridge/Symfony/Routing/ApiLoader.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Routing/IriConverter.php Show resolved Hide resolved
src/OpenApi/Factory/OpenApiFactory.php Outdated Show resolved Hide resolved
src/OpenApi/Factory/OpenApiFactory.php Outdated Show resolved Hide resolved
src/Operation/Factory/SubresourceOperationFactory.php Outdated Show resolved Hide resolved
@soyuka soyuka merged commit c86bb1d into api-platform:master Nov 30, 2020
@soyuka soyuka deleted the feature/improve-identifiers branch November 30, 2020 17:30
@soyuka soyuka restored the feature/improve-identifiers branch November 30, 2020 17:30
@bendavies
Copy link
Contributor

bendavies commented Jan 17, 2021

@soyuka this broke custom identifiers, InvalidIdentifierException is now thrown in OperationDataProviderTrait::extractIdentifiers.
example, which works on 2.5.9, but broken on 2.6.0 beta1

/**
 * @ApiResource(
 *     itemOperations={
 *         "get": {
 *             "method": "GET",
 *             "path": "/customers/{id}"
 *         }
 *     }
 * )
 */
Class Customer
{
    /**
     * @ApiProperty(identifier=false)
     *
     * @ORM\Id
     * @ORM\Column(type="uuid")
     * @ORM\GeneratedValue(strategy="NONE")
     */
    protected UuidInterface $id;

    /**
     * @ApiProperty(identifier=true)
     *
     * @ORM\Column(type="string")
     */
    protected string $externalId;
}

/customers/foo won't work now.

EDIT:

changing the operation path from "path": "/customers/{id}" to "path": "/customers/{externalId}" fixes it, is this intentional or a BC break?

@soyuka
Copy link
Member Author

soyuka commented Jan 22, 2021

@bendaviesthe change is intentional I've taken great care to try and make the change as soft as possible. I may have forgotten something here. What you can do is:

/**
 * @ApiResource(
 *     itemOperations={
 *         "get": {
 *             "method": "GET",
 *             "path": "/customers/{id}",
 *             "identifiers": {"id": {Customer::class, "externalId"}}
 *         }
 *     }
 * )
 */
Class Customer
{
    /**
     * @ApiProperty(identifier=false)
     *
     * @ORM\Id
     * @ORM\Column(type="uuid")
     * @ORM\GeneratedValue(strategy="NONE")
     */
    protected UuidInterface $id;

    /**
     * @ApiProperty(identifier=true)
     *
     * @ORM\Column(type="string")
     */
    protected string $externalId;
}

Did it at least throw an error helping you fix it?

@bendavies
Copy link
Contributor

@soyuka yes, this error was thrown:

throw new InvalidIdentifierException(sprintf('Parameter "%s" not found', $parameterName));

thanks for the code sample using identifers. i fixed it using "path": "/customers/{externalId}" but yours seems nicer!

@bendavies
Copy link
Contributor

hm what if i remove my path completely? will that work too?
i notice your identifiers configuration isn't covered here: https://api-platform.com/docs/core/identifiers/#identifiers

@soyuka
Copy link
Member Author

soyuka commented Jan 22, 2021

Yes removing the path should work!

Indeed identifiers are not documented yet I need to find some time, in the meantime you can use https://github.com/api-platform/core/blob/master/docs/adr/0001-resource-identifiers.md as reference.

@tiavina-mika
Copy link

    /**
     * @ORM\Column(type="string", length=255)
     * @ApiProperty(identifier=true)
     */
    private $slug;

works fine.

But what if I need both id and slug as identifiers?
For exemple, I want both endpoints

  • /api/articles/{id}
  • /api/articles/slug/{slug}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Produce "Invalid identifier value or configuration" error when no "setId()" method on entity
5 participants