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

Annotations for routing configuration #301

Merged
merged 5 commits into from Feb 7, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Feb 5, 2019

This PR is regarding issue #296 . Please review and check it.

composer.json Outdated Show resolved Hide resolved
@@ -1,10 +1,7 @@
librecores_site_home:
path: /
defaults: { _controller: LibrecoresSiteBundle:Default:home }
resource: '@LibrecoresSiteBundle/Controller/'

This comment has been minimized.

Copy link
@agathver

agathver Feb 5, 2019

Collaborator

you can actually write:

librecores_site:
    resource: '@LibrecoresSiteBundle/Controller/'
    type: annotation

Once. Repetition is redundant

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Feb 5, 2019

Author Collaborator

@agathver I think that would do

@agathver agathver changed the title Annotations Annotations for routing configuration Feb 5, 2019
Copy link
Contributor

imphil left a comment

Thanks for this PR @aquibbaig!

@@ -40,8 +43,10 @@ public function homeAction(Request $request)
/**
* XXX: add caching for static pages
* see http://symfony.com/doc/current/bundles/SensioFrameworkExtraBundle/annotations/cache.html
*
* @Route("/{page}", name="librecores_site_page", requirements={"page"=".+"})

This comment has been minimized.

Copy link
@imphil

imphil Feb 6, 2019

Contributor

The comment # allow / inside path was pretty helpful to understand why the requirements option is added. Please restore that. Can you also add spaces before/after the = to make it requirements = { etc.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Feb 6, 2019

Author Collaborator

@imphil Thank you for suggesting! I have made the changes. Please review them.

@imphil imphil merged commit e6130c9 into librecores:master Feb 7, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Feb 7, 2019

Thanks @aquibbaig! I've squashed and merged your changes. Hooray on your first contribution to this repository!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.