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

[Cookbook][Routing] Update custom_route_loader.rst #4790

Merged
merged 4 commits into from
May 23, 2015

Conversation

xelaris
Copy link
Contributor

@xelaris xelaris commented Jan 7, 2015

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets

Just some tweaks (including "Acme\DemoBundle" => "AppBundle").

While reading this chapter I wonder, if

A custom route loader allows you to add routes to an application without
including them, for example, in a YAML file. [...] This may be especially important when you want
to make the bundle reusable, or when you have open-sourced it as this would
slow down the installation process and make it error-prone.

may raise false expectations. Since also with a route loader it is still required to add something to the routing.yml (or xml). I don't see the benefit of using a custom route loader in a reusable bundle over importing a YAML file from a third-party bundle like FOSUserBundle. It makes totally sense for something like the FOSRestBundle does (generating routes based on conventions), but the chapter introduction sounds, like a Bundle could load its routes without manual tweaks to the config. If I am not wrong, this isn't possible, isn't it?

@wouterj
Copy link
Member

wouterj commented Jan 16, 2015

I agree with you, I see no benefit of a custom routing loader.

Some changes in this PR might conflict with #4740. Could you please check that and remove the ones that will conflict?

@xelaris
Copy link
Contributor Author

xelaris commented Jan 16, 2015

@wouterj The file cookbook/routing/custom_route_loader.rst is not part of #4740, isn't it?

@wouterj
Copy link
Member

wouterj commented Jan 16, 2015

Indeed, my bad. I thought it changed all cookbook articles.


.. note::

There are many bundles out there that use their own route loaders to
accomplish cases like those described above, for instance
`FOSRestBundle`_, `JMSI18nRoutingBundle`_, `KnpRadBundle`_ and `SonataAdminBundle`_.
`FOSRestBundle`_, `JMSI18nRoutingBundle`_, `KnpRadBundle`_ and
`SonataAdminBundle`_.
Copy link
Member

Choose a reason for hiding this comment

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

we might want to add AsseticBundle in the list actually

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 rather see a shorter list than making it longer

@weaverryan
Copy link
Member

@xelaris this looks great - nice work! But you're right - the first 2 paragraphs (especially the first) is terrible! I think we should mention FOSRestBundle as we do now, and make a new intro centered around it as one example.

Also, it's out of scope, but I think we should just show the class extending Loader in the first example. That's much easier and more powerful. Then we can have a note that says that you can simply implement LoaderInterface instead of extending Loader if you want.

@xelaris Can you at least take a shot at updating the first paragraph for us?

@xabbuh
Copy link
Member

xabbuh commented Mar 8, 2015

ping @xelaris :)

@xelaris
Copy link
Contributor Author

xelaris commented Mar 9, 2015

Thank you for the ping @xabbuh. I will try to go on with this on the weekend.

@xelaris
Copy link
Contributor Author

xelaris commented Mar 29, 2015

@weaverryan Thank you for the feedback. I have changed the first two paragraphs and tried to make it more clear.
In addition I have changed the code example to extend from Loader as you suggested.

@xelaris
Copy link
Contributor Author

xelaris commented Apr 28, 2015

ping @weaverryan

have to create an ``extraAction`` method in the ``ExtraController``
of the ``AppBundle``::

namespace AppBundle\Controller;
Copy link
Member

Choose a reason for hiding this comment

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

you should add the filename comment above this:

// src/AppBundle/Controller/ExtraController.php

@xelaris
Copy link
Contributor Author

xelaris commented Apr 29, 2015

Thanks @xabbuh. I have added filename comments to all the php code blocks.

@wouterj
Copy link
Member

wouterj commented May 22, 2015

I don't think anything is missing here. Flagged it with Finished :)

@xabbuh
Copy link
Member

xabbuh commented May 22, 2015

I agree 👍

@weaverryan
Copy link
Member

This looks really awesome - I have no changes. Thanks so much Alexander!

@weaverryan weaverryan merged commit 08b1527 into symfony:2.3 May 23, 2015
weaverryan added a commit that referenced this pull request May 23, 2015
…aris)

This PR was merged into the 2.3 branch.

Discussion
----------

[Cookbook][Routing] Update custom_route_loader.rst

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

Just some tweaks (including "Acme\DemoBundle" => "AppBundle").

While reading this chapter I wonder, if

> A custom route loader allows you to add routes to an application without
 including them, for example, in a YAML file. [...] This may be especially important when you want
 to make the bundle reusable, or when you have open-sourced it as this would
 slow down the installation process and make it error-prone.

may raise false expectations. Since also with a route loader it is still required to add something to the routing.yml (or xml). I don't see the benefit of using a custom route loader in a reusable bundle over importing a YAML file from a third-party bundle like FOSUserBundle. It makes totally sense for something like the FOSRestBundle does (generating routes based on conventions), but the chapter introduction sounds, like a Bundle could load its routes without manual tweaks to the config. If I am not wrong, this isn't possible, isn't it?

Commits
-------

08b1527 Add filename comments to code blocks
fe1a574 Change code example to extend from Loader class
bd6e3f3 Rewrite first paragraph
46d10a2 [Cookbook][Routing] Update custom_route_loader.rst
@xelaris xelaris deleted the custom_route_loader-tweaks branch October 31, 2022 12:17
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.

5 participants