Skip to content

Update routing.rst #3263

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
Closed

Update routing.rst #3263

wants to merge 1 commit into from

Conversation

fatmuemoo
Copy link
Contributor

Added a note to provide extra guidance when naming routes

Added a note to provide extra guidance when naming routes
@@ -68,6 +68,13 @@ The route is simple:

return $collection;

.. note::

When defining routes, the key (e.g. ``blog_show``) is meaningless.
Copy link
Member

Choose a reason for hiding this comment

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

it's not meaningless, you use it to reference to the route (for instance when generating a route in a template or controller)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I concur. "Meaningless" is the wrong word. "Arbitrary" would be a better way of describing the route name.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this to the last sentence of the next paragraph - so right after in your controller (keep reading). - how about:

The blog_show is the internal name of the route, which just doesn't have any meaning yet and just needs to be unique. Later, you'll use it to generate URLs.

I think if we do this (feel free to re-word), then we're in good shape!

Copy link
Member

Choose a reason for hiding this comment

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

We should at least avoid the first person perspective.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - updated my example above!

Copy link
Member

Choose a reason for hiding this comment

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

@fatmuemoo What do you think about my proposed change above? Can you update your PR? If not, just let me know and someone else can open a new PR with the tweak.

Cheers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, thanks

Copy link
Member

Choose a reason for hiding this comment

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

@fatmuemoo :) - ok - but can you also update your pull request with the suggested changes? I can also update it for you if you'd like!

Cheers!

@wouterj
Copy link
Member

wouterj commented May 21, 2014

Replaced by #3857

@wouterj wouterj closed this May 21, 2014
weaverryan added a commit that referenced this pull request May 27, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

Added little bit information about the route name

This PR replaces #3263

| Q   | A
| --- | ---
| Doc fix? | no
| New docs? | yes
| Applies to | all
| Fixed tickets | -

Commits
-------

90e654b Applied comments
66d60d2 Update routing.rst
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.

4 participants