Skip to content

Conversation

javiereguiluz
Copy link
Member

The last remaining big features are:

* @Route("/", defaults={"page": "1", "_format"="html"}, methods={"GET"}, name="blog_index")
* @Route("/rss.xml", defaults={"page": "1", "_format"="xml"}, methods={"GET"}, name="blog_rss")
* @Route("/page/{page}", defaults={"_format"="html"}, requirements={"page": "[1-9]\d*"}, methods={"GET"}, name="blog_index_paginated")
* @Route("/page/{page<[1-9]\d*>}", defaults={"_format"="html"}, methods={"GET"}, name="blog_index_paginated")
Copy link
Member

Choose a reason for hiding this comment

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

I would even change it to \d*+. Using possessive quantifiers in routing requirements makes matching faster

* Displays a form to edit an existing Post entity.
*
* @Route("/{id}/edit", requirements={"id": "\d+"}, methods={"GET", "POST"}, name="admin_post_edit")
* @Route("/{id<\d+>}/edit",methods={"GET", "POST"}, name="admin_post_edit")
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using a possessive quantifier here (\d++), to improve the route matching performance (Symfony generates possessive quantifiers for its builtin requirements, but custom ones need to do that themselves, as using non-possessive quantifiers can still be a valid use case)

*
* @ORM\Column(type="string", unique=true)
* @Assert\NotBlank()
* @Assert\Length(min = 2, max = 50)
Copy link
Member

Choose a reason for hiding this comment

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

The existing annotations are not putting spaces around the =

@javiereguiluz
Copy link
Member Author

@stof I've merged without making the changes in the routing requirements ... but I've opened #818 to discuss about your proposal. Cheers!

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.

2 participants