Route names must be unique #9237

saeideng · 2016-08-09T08:49:17Z

No description provided.

stickler-ci · 2016-08-09T08:49:41Z

src/Routing/RouteCollection.php

@@ -77,6 +77,12 @@ public function add(Route $route, array $options = [])

        // Explicit names
        if (isset($options['_name'])) {
+            if(isset($this->_named[$options['_name']])){


Expected 1 space after IF keyword; 0 found
Expected "if (...) {\n"; found "if(...){\n"
Expected 1 space after closing parenthesis; found 0

htstudios · 2016-08-09T09:18:27Z

However I am not sure I agree.

E.g. I was yesterday planning a flexible (plugin-ized) app in my head and thought "okay why not route all without AND with plugin prefixes, if there is a collision (non unique routes) the first route will match. The later defined routes will go with plugin prefixes.

So the docs should clearly state how to catch the Exception to allow this. And/or there should be some way to allow collisions (priority then taking effect)

codecov-io · 2016-08-09T09:39:03Z

Current coverage is 94.93% (diff: 83.33%)

Merging #9237 into master will increase coverage by 0.03%

@@             master      #9237   diff @@
==========================================
  Files           369        410    +41   
  Lines         27035      28030   +995   
  Methods        3241       3357   +116   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          25656      26610   +954   
- Misses         1379       1420    +41   
  Partials          0          0

Powered by Codecov. Last update 3d3e354...673550e

lorenzo · 2016-08-09T10:46:30Z

src/Routing/RouteCollection.php

@@ -77,6 +77,12 @@ public function add(Route $route, array $options = [])

        // Explicit names
        if (isset($options['_name'])) {
+            if (isset($this->_named[$options['_name']])) {
+                throw new MissingRouteException([


This would no be a missing route exception, I think

DuplicateRouteException?

@lorenzo
there is not something else,i agree with you
or should be add a new exception into Core

A new exception sounds better

Ye, that's what I meant by my comment above. Sorry for not being clear

Also tests please 😄

DuplicateRouteException is good?
Or other name?

On a second thought I think that name can be confusing...
DuplicateRouteNameExceptionis more clear

This case: DuplicateNamedRouteException

If in general the Router should become more strict now allowing to declare the same Route twice DuplicateRouteException

👍 to DuplicateNamedRouteException.

stickler-ci · 2016-08-13T19:34:40Z

src/Routing/RouteCollection.php

@@ -15,6 +15,7 @@
 namespace Cake\Routing;

 use Cake\Routing\Exception\MissingRouteException;
+use Cake\Routing\Exception\DuplicateNamedRouteException;


Use classes must be in alphabetical order. Was expecting Cake\Routing\Exception\MissingRouteException

markstory · 2016-08-14T02:51:33Z

src/Routing/RouteCollection.php

+            if (isset($this->_named[$options['_name']])) {
+                throw new DuplicateNamedRouteException([
+                    'url' => $options['_name'],
+                    'message' => 'A named route was found for "%s" that is already used, route names must be unique across your entire application.'


Do you need the message in both places? If we don't need both copies of the message, can we remove one?

Yes , you can

markstory · 2016-08-14T02:53:01Z

src/Template/Error/duplicate_named_route.ctp

+foreach (Router::routes() as $route):
+	if(isset($route->options['_name']) && $url===$route->options['_name']){
+		echo '<tr class="error">';
+	}else{


You're missing spaces around else and if here. Also try to avoid using a mix of { and alternative-syntax.

Do we already have a code guideline for what/when to use alternative syntax in template files (such as, use regular syntax at the head if you pre-process a lot of data, use alternative syntax in the body of a template only?)

Does it make sense to add such a guideline if it is not there?

There is a section in the coding standards

jadb · 2016-08-14T22:51:43Z

src/Routing/Exception/DuplicateNamedRouteException.php

+    /**
+     * {@inheritDoc}
+     */
+    protected $_messageTemplate = 'A named route was found for "%s" that is already used, route names must be unique across your entire application.';


Can this be made shorter? My suggestion:

Named routes must be unique; duplicate found ("%s").

Duplicate named route "%s" but must be unique.

@jadb maybe,but big message can help to developer even if found that in error.log file
some times developers will added route runtime(when site is run) and big message can helpful to that

@markstory please see new commit

Am not against long ones, neither am I an expert in english. but "was found" and "is already used" mean the same thing, no? And "across your entire application", not sure is necessary.

I'd prefer a long message that contains what are the 2 conflicting routes. If we don't tell the user what is the other route with the same name, then we are not really helping solve the issue

Duplicate named route x conlicts with y?

Would displaying the template of the conflicting route be enough context for folks?

Isn't this for named routes only? Then I'd expect the name, no? Or is that what you mean by "template" @markstory?

The template of a route would be something like /articles/:year/:month/*. It is the first argument to connect().

I think it would need to show the template of both conflicting routes.

saeideng · 2016-08-17T09:24:44Z

src/Template/Error/duplicate_named_route.ctp

+}
+foreach (Router::routes() as $route) :
+    if (isset($route->options['_name']) && $url === $route->options['_name']) :
+        echo '<tr class="error">';


@lorenzo conflicting named routes show with this line

Would't it be better to have that information in the exception object instead of fetching it dynamically here?

how?
currently we tell that with "%s" in message

You can pass the info to the exception constructor or with a setter.

i'm sorry
I did not understand what you mean

I've moved the duplicate route into a table all of its own. I don't think we need the entire route list when we know which one is the duplicate.

Throw exceptions when duplicate named routes are connected. Refs #9237

Instead of relying on row highlighting, bubble the duplicate route into its own table at the top. Refs #9237

markstory · 2016-08-19T02:04:12Z

Merged into master in a7ac286

Route names must be unique

72cb59f

stickler-ci reviewed Aug 9, 2016
View reviewed changes

CS and other

e95c6e3

lorenzo reviewed Aug 9, 2016
View reviewed changes

markstory added this to the 3.2.14 milestone Aug 9, 2016

markstory added routing enhancement labels Aug 9, 2016

add tests and DuplicateRouteException

94571c5

markstory modified the milestones: 3.2.14, 3.3.1 Aug 13, 2016

rename to DuplicateNamedRouteException and tests

34aa83e

stickler-ci reviewed Aug 13, 2016
View reviewed changes

CS & version

45bb047

markstory reviewed Aug 14, 2016
View reviewed changes

fix CS/and remove extra test function

673550e

jadb reviewed Aug 14, 2016
View reviewed changes

markstory self-assigned this Aug 15, 2016

saeideng reviewed Aug 17, 2016
View reviewed changes

markstory added a commit that referenced this pull request Aug 19, 2016

Merge branch 'duplicate-route' into master.

a7ac286

Throw exceptions when duplicate named routes are connected. Refs #9237

markstory added a commit that referenced this pull request Aug 19, 2016

Make duplicate route simpler to find.

01b32f1

Instead of relying on row highlighting, bubble the duplicate route into its own table at the top. Refs #9237

markstory closed this Aug 19, 2016

mirko-pagliai mentioned this pull request Aug 20, 2016

Rewrite route with name #9312

Closed

Route names must be unique #9237

Route names must be unique #9237

Conversation

saeideng commented Aug 9, 2016

Choose a reason for hiding this comment

htstudios commented Aug 9, 2016

codecov-io commented Aug 9, 2016 • edited

Current coverage is 94.93% (diff: 83.33%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xymanek Aug 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadb Aug 14, 2016 • edited

Choose a reason for hiding this comment

htstudios Aug 14, 2016 • edited

Choose a reason for hiding this comment

saeideng Aug 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzo Aug 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markstory commented Aug 19, 2016

codecov-io commented Aug 9, 2016 •

edited

Xymanek Aug 9, 2016 •

edited

jadb Aug 14, 2016 •

edited

htstudios Aug 14, 2016 •

edited

saeideng Aug 15, 2016 •

edited

lorenzo Aug 18, 2016 •

edited