8000 Allow a shortcut for UUID when defining requirements by Nyholm · Pull Request #18599 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Allow a shortcut for UUID when defining requirements #18599

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

Closed
wants to merge 1 commit into from

Conversation

Nyholm
Copy link
Member
@Nyholm Nyholm commented Apr 20, 2016
Q A
Branch? "master"
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR TODO

Related to sensiolabs/SensioFrameworkExtraBundle#412


I use an uuid (v4) in my URL to identify an entity (like you always should). This is a recommended security measure if you compare with just using an auto incremental numeric id (which is standard for a Doctrine/Mysql setup). I want of course make sure that my routes are properly defined and that I have a requirement on all my variables. So I write something like this:

/**
* @Route("/show/{uuid}", name="show_user", requirements={"uuid" = "[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}"})
*/
public function showAction(User $user)

The regex for the uuid is quite long and horrible to write/remember. This PR allows you to write a shortcut for that ugly regex. So instead you would write:

/**
 * @Route("/show/{uuid}", name="show_user", requirements={"uuid" = "\uuid"})
 */
public function showAction(User $user)

This will also work in Yaml, XML and PHP.

BC break?

This is not a BC break because nobody will have the string \uuid as an requirement. If you would write \uuid or \uxxx in the requirements you would get an error like:

Warning: preg_match(): Compilation failed: PCRE does not support \L, \l, \N{name}, \U, or \u at offset 17

@javiereguiluz
Copy link
Member

I'm 👎 for this change. It looks like a "hack", it introduces a special use case (why UUID and not other shortcuts) and it's incomplete (which UUID version are we talking about? Because there are a lot of them).

Using UUIDs, like using autoincremental IDs, has some pros and some cons. I'm afraid this horrible regexp is one of the cons (the increase of the index sizes and the performance penalty are other cons too).

@javiereguiluz
Copy link
Member

@iltar do you know if your cool new controller argument resolver would help @Nyholm to make these UUIDs a bit more friendly to use?

@Nyholm
Copy link
Member Author
Nyholm commented Apr 20, 2016

Thank you for the feedback.

why UUID and not other shortcuts

I would welcome other shortcuts as well.

which UUID version are we talking about? Because there are a lot of them

There are 5 versions and they all have the same format. See https://en.wikipedia.org/wiki/Universally_unique_identifier#Variants_and_Versions

Using UUIDs, like using autoincremental IDs, ...

You should not replace the auto incremental IDs with UUID on your entities. You should use both. You should still use ID in your indexes etc but use the UUID in your URLs. Then you will have the best of two worlds.

@lyrixx
Copy link
Member
lyrixx commented Apr 20, 2016

Just for your information, we have solved this issue with a parameter in the container:

# config.yml
parameters:
    uuid_regex: '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}'

and the usage in a controller:

    /**
     * @Route(
     *   path="/{id}",
     *   requirements = {
     *     "id": "%uuid_regex%"
     *   }
     * )
     *

@stof
Copy link
Member
stof commented Apr 20, 2016

I did the same than @lyrixx

and I also vote -1 about adding such hack in Symfony

@aitboudad
Copy link
Contributor

In fact, it will be a nice example to add it in the doc

@linaori
Copy link
Contributor
linaori commented Apr 20, 2016

I agree that a doc example might be nice here. @javiereguiluz you can use the doctrine paramconverter to achieve this exact behavior (transforming the uuid into an entity), which will provide a more generic solution.

It would of course be possible to make an argument resolver for a specific Object, but this would only be deemed valuable if you have additional logic in the resolving (security perhaps).

@stof
Copy link
Member
stof commented Apr 20, 2016

@iltar we are not talking about converting the UUID to an entity in this issue. We are talking about putting a routing requirement for a UUID.

@linaori
Copy link
Contributor
linaori commented Apr 20, 2016

@stof if it's about matching, you can throw an exception when you cannot find an entity matching the UUID given, that's what I mean. Matching routes or throwing exceptions is a slightly different thing though.

@stof
Copy link
Member
stof commented Apr 20, 2016

@iltar we are not talking about matching that the UUID has an entity. We are talking about matching the URL only when the placeholder is a UUID (instead of having to accept any string, which can create issue by shadowing another route). Defining the regex matched by the placeholder is part of the route matching.

@lyrixx
Copy link
Member
lyrixx commented Apr 20, 2016

@iltar More over, when you are using postgres, if the type is uuid, and a user try to enter foobar, or 1 or anything that is not a real uuid postgres will raise an exception, and so dbal and so the the orm and so symfony and so the application. So it leads to a 500 and a log message :/ So definitively, It should be handled at the routing level.

@linaori
Copy link
Contributor
linaori commented Apr 20, 2016

In that case the ArgumentResolver will not be of any good

@Nyholm
Copy link
Member Author
Nyholm commented Apr 20, 2016

More over, when you are using postgres, if the type is uuid, and a user try to enter foobar, or 1 or anything that is not a real uuid postgres will raise an exception, and so dbal and so the the orm and so symfony and 8000 so the application. So it leads to a 500 and a log message :/ So definitively, It should be handled at the routing level.

You will also get the exception when using Doctrine and MySql when you use the Ramsey uuid-doctrine lib.

@javiereguiluz
Copy link
Member

I'm closing this because it received some -1 votes for the Core Team and because @lyrixx provided a clever and simple solution. @Nyholm although this time your proposal didn't make it, thanks for your support and for proposing ways to improve Symfony!

@Nyholm
Copy link
Member Author
Nyholm commented Apr 21, 2016

Thank you for all the feedback. I'll go back to the original PR at the FrameworkExtraBundle.

I really like @lyrixx solution. I did not know you could use parameters in the requirement.

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

Successfully merging this pull request may close these issues.

7 participants
0