Skip to content

[HttpKernel] Add the UidValueResolver argument value resolver #44665

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 24, 2022
Merged

[HttpKernel] Add the UidValueResolver argument value resolver #44665

merged 1 commit into from
Feb 24, 2022

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Dec 16, 2021 •
edited
Loading

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets #41690
License MIT
Doc PR -

This feature adds an argument value resolver for UIDs.

Before:

#[Route(path: '/token/{token}')]
public function __invoke(string $token): Response
{
    $token = UuidV4::fromRfc4122($token);
    // ...
}

After:

#[Route(path: '/token/{token}')]
public function __invoke(UuidV4 $token): Response
{
    // ...
}

By default, all formats are accepted because the resolver uses the fromString() method from the target UID class.

It's possible to restrict to one format or a combination of formats by using a route parameter requirement, for example:

#[Route(path: '/token/{token}', requirements: ['id' => Requirement::UUID])]
public function __invoke(UuidV4 $token): Response
{
    // ...
}

@carsonbot carsonbot added this to the 6.1 milestone Dec 16, 2021
fancyweb
fancyweb commented Dec 16, 2021
View reviewed changes
...mfony/Bundle/FrameworkBundle/Tests/Functional/Bundle/TestBundle/Controller/UidController.php
use Symfony\Component\Uid\UuidV4;
use Symfony\Component\Uid\UuidV6;

class UidController
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check this file for different use cases.

@fancyweb fancyweb force-pushed the uid/arg-resolver branch 2 times, most recently from 0188adb to 6712733 Compare December 16, 2021 16:48
@aurimasniekis
Copy link
Contributor

Nice, I was just thinking of making something like this 👍 I am just trying to figure out now how is the best way to make route parameter requirements based on Uid

@fancyweb fancyweb force-pushed the uid/arg-resolver branch 2 times, most recently from b2c1b7f to fd22d35 Compare December 17, 2021 08:23
@nicolas-grekas
Copy link
Member

I am just trying to figure out now how is the best way to make route parameter requirements based on Uid

That's a good question actually. If we figure out how to make this, we might be able to remove the proposed AsUid attribute and move the check to the router level.

Any idea how to do this?

@nicolas-grekas
Copy link
Member

Now I'm wondering: what about removing the "format" configuration setting and attribute, and instead accept only the canonical representation?
Eg here, if one prefers representing their UUID as base58, shouldn't they create a custom class that extends Uuid?
Adding config options is increasing complexity at all levels, we'd better be sure that this is needed before doing it.

@aurimasniekis
Copy link
Contributor

Any idea how to do this?

I haven't yet looked into the router, but I am wondering is there some kind of parser level for parameters before generating URL matchers? If so we could add some extension in that point which would mark that param as UID

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 17, 2021 •
edited
Loading

We could define a uid_base58 parameter and use {id<%uid_base58%>}. Or we could improve the routing configuration to add support for predefined requirements (aka aliased regexps).

If this looks like a useful path forward, we could then simplify this PR and make the resolver accept any format.

I think that's my current preference (and we could define these uid_* parameters or do this improvement of the routing system in a separate PR.)

@aurimasniekis
Copy link
Contributor

My previous implementation was basically like you mention parameter + argument resolver, but I was planning to look into something nicer

@fancyweb fancyweb force-pushed the uid/arg-resolver branch 2 times, most recently from 49c3f64 to 4b61f8e Compare December 20, 2021 15:31
@fancyweb fancyweb mentioned this pull request Feb 23, 2022
Merged
@fancyweb fancyweb changed the title [Uid] Add the UidValueResolver argument value resolver [HttpKernel] Add the UidValueResolver argument value resolver Feb 23, 2022
@carsonbot carsonbot changed the title [HttpKernel] Add the UidValueResolver argument value resolver [HttpKernel][Uid] Add the UidValueResolver argument value resolver Feb 23, 2022
@fancyweb fancyweb removed the Uid label Feb 23, 2022
@carsonbot carsonbot changed the title [HttpKernel][Uid] Add the UidValueResolver argument value resolver [HttpKernel] Add the UidValueResolver argument value resolver Feb 23, 2022
@fancyweb fancyweb force-pushed the uid/arg-resolver branch 2 times, most recently from 11ef7d9 to 785c6ef Compare February 23, 2022 16:16
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas mentioned this pull request Feb 24, 2022
Closed
@nicolas-grekas nicolas-grekas merged commit 8a31425 into symfony:6.1 Feb 24, 2022
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
fabpot added a commit that referenced this pull request Apr 15, 2022
@fabpot
feature #45528 [Routing] Add Requirement, a collection of universal r…
2a38810 
…egular-expressions constants to use as route parameter requirements (fancyweb)

This PR was merged into the 6.1 branch.

Discussion
----------

[Routing] Add Requirement, a collection of universal regular-expressions constants to use as route parameter requirements

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Ref #44665.

We need a way to easily reference some universal complicated regexes in routes parameters requirements, for example:
```php
#[Route(path: '/users/{id}', requirements: ['id' => Requirement::UUID])]

#[Route(path: '/users/{id<'.Requirement::UID_BASE58.'>}')]
```

What about having a collection in the routing component itself? I'm sure there are other common cases we would add here.

Commits
-------

f49cb6a [Routing] Add Requirement, a collection of universal regular-expression constants to use as route parameter requirements
@derrabus derrabus mentioned this pull request Sep 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@ogizanagi ogizanagi ogizanagi approved these changes

Assignees
No one assigned
Labels
Feature HttpKernel Status: Reviewed
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.
5 participants
@fancyweb @aurimasniekis @nicolas-grekas @ogizanagi @carsonbot