8000 Error when using the same route name twice in XML or YAML file · Issue #14512 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Error when using the same route name twice in XML or YAML file #14512

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.

Sign up for GitHub

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

Jump to bottom
Closed
beberlei opened this issue Apr 30, 2015 · 21 comments
Closed

Error when using the same route name twice in XML or YAML file #14512

beberlei opened this issue Apr 30, 2015 · 21 comments

Comments

@beberlei
Copy link
Contributor

I suppose you already had this before as well, copying a route and forgetting to rename it. The old route gets overwritten and you might not even notice it until its too late.

How about throwing an Exception when the same route name is defined twice in the same YAML or XML file to increase developer experience?
@inso
Copy link
inso commented Apr 30, 2015

#12561 (comment)

@ghost
Copy link
ghost commented May 1, 2015

@xabbuh wrote:
> Nonetheless, it could indeed be an improvement to trigger a warning or something like that when a
> route will be added to a RouteCollection that already contains a route with the same name.

Will there be a warning now? I think it would be good to add this warning if it not exists.

@sstok
Copy link
Contributor
sstok commented May 5, 2015

Disallowing to overwrite a route is a major BC and also problems for some that use this feature.
I think logging that a route is overwritten is the best solution, setting overwrite: true like Tobion suggested only works half as it moves the problem (what if you used this directive and then forget that you used it :) ).

@fabpot
Copy link
Member
fabpot commented May 5, 2015

That's really a limitation of YAML, so there is nothing we can do as mentioned by @inso
I'm in favor as closing this one as a won't fix.

@SoboLAN
Copy link
SoboLAN commented May 6, 2015

I think no one should rely on this behaviour. I can't think of any relevant use case where having multiple routes with the same name is valid. If you absolutely need to have something like that, then your design is probably wrong and it needs to be refactored.

In Symfony\Component\Routing\RoutingCollection::addCollection() we could probably check if the $routes field already has a key with the same name (which is also the route name). If it does, then maybe we can somehow log it (although I don't see any access to a logger there) or throw an exception and catch it somewhere where it can be logged.

Come on guys, this is a good concept:

  • It falls in line with Symfony's relative strictness regarding configuration.
  • Think for example what would happen if you had dozens or even hundreds of routes and duplicate naming happens. You could end up spending hours or even days figuring out what is going on.
  • Duplicate services are not allowed. And it's like that for a reason: because it causes ambiguity and having duplicate names makes no sense. It should be the same for routes.

@flip111
Copy link
Contributor
flip111 commented May 11, 2015

I'm in favor of improving this. The YAML parser does not give a warning, when it can

Developer experience is very important. And i think it does not even break the YAML spec, but infact enhance it ! Implementation wise it should be easy to do something about this,

@fabpot
Copy link
Member
fabpot commented May 11, 2015

Adding a warning would actually degrade the experience for people who uses this documented feature of YAML. It's not "bad" to override a key in YAML.

@Tobion
Copy link
Contributor
Tobion commented May 11, 2015

@fabpot overwriting makes sense when combining multiple YAML files. But in a single file, there is not overwriting. The first key wins and the rest is ignored. See #10902

@flip111
Copy link
Contributor
flip111 commented May 12, 2015

Is one file one YAML node?

@ghost
Copy link
ghost commented May 12, 2015

@Tobion This sounds like a bug if it occurs in a single file and should output a WARNING or be changed (let's say use the last?).

@SoboLAN
Copy link
SoboLAN commented May 13, 2015

@Tobion why does it make sense when combining multiple YAML files ? How can you know which file overwrites which ? Are the files containing the routes loaded in a predictable order in the case of multiple bundles ?

@Tobion
Copy link
Contributor
Tobion commented May 13, 2015

@SoboLAN yes the order you import them.

@thms
Copy link
thms commented Sep 8, 2015

Here is a place where this is really important:
Using FOSUserBundle the routes are defined in xml that is imported in routing.yml.
Since the currently implementation of requesting password reset exposes whether the email address exists in the database, the behaviour of the sendEmail action needs to be changed.
Option one: override the route and send this one action to my own controller. This is preferred, but does not work.
Option two: fork FOSUser and fix it.
Option three: unpack all resetting routes from FOSUser into routing.yml, which is really not a great option either.

So the proper solution would be to allow multiple routes with the same name and have a clear policy of which one wins, irrespective of the parser / loader used.

@stof
Copy link
Member
stof commented Sep 8, 2015

Option one: override the route and send this one action to my own controller. This is preferred, but does not work.

@thms are you sure this does not work ? redefining routes in different files should be working AFAICT.

In any case, overwriting stuff from a third-party bundle is off-topic here. this issue is about route names in a single file.

@albe
Copy link
albe commented Sep 13, 2015

If I may give my input: It is really a no-go to silently ignore declarations (ie. user input). Also, the spec clearly states that duplicate keys are supposed to be considered an error and, at least in 1.1 spec, that an according warning should be issued. This is currently not happening as of #10902 and is a source of painfully hard to debug errors.

And yes, that has basically already been rejected with #11538, but I dare to oppose the "overhead for too little gain" resolution. Filling an internal array of spec-violations and making it accessable through the parser class is really easy opposed to the possible problems that may result from silent ignoring such. It should at least be the decision of the application if it can really ignore such illformatting or has to inform the user/developer.

@Tobion @fabpot @wouterj

@kdambekalns
Copy link

I second what @albe wrote. And dare to suspect most applications reading YAML will cache the result in a production context - so a little overhead would not hurt.

@javiereguiluz
Copy link
Member
javiereguiluz commented May 28, 2016 •
edited
Loading

For what is worth, according to the Yaml 1.2 Spec (which is the one used by Symfony) keys must be unique:

JSON requires that mappings keys merely SHOULD be unique, while YAML insists they MUST be.

Source: http://www.yaml.org/spec/1.2/spec.html - Section 1.3. Relation to JSON

@javiereguiluz javiereguiluz mentioned this issue Aug 4, 2016
Closed
@javiereguiluz
Copy link
Member

Could we close this issue as fixed now that Symfony has deprecated "silently ignoring duplicate keys" and this will throw a ParseException in Symfony 4.0?

@xabbuh xabbuh closed this as completed Oct 6, 2016
@curry684
Copy link
Contributor
curry684 commented May 2, 2017

@javiereguiluz will that also include naming conflicts between imported routes? That's a far more frequent source for hard to find bugs in my experience.

@stof
Copy link
Member
stof commented May 3, 2017

@curry684 being able to override a route which is already defined is considered as a feature, so we cannot forbid it

@curry684
Copy link
Contributor
curry684 commented May 3, 2017

@stof I'm aware that it wasn't "fixed" before for BC reasons. It's however a highly frustrating experience for developers to accidentally overwrite a route name in a huge project with several local bundles and partial routing files and then subtly break existing functionality.

Hence my question whether, as 4.0 also fixes the broken behavior inside YAML files, that wouldn't be a good opportunity to also fix the cross-file issue causing far more obscure bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0