Yes, we're well past API freeze. That said...

Tonight I was futzing around with Symfony because what else are you going to do when you have insomnia? I discovered that all of the examples from the "official" Symfony docs for the routing component use "path" and not "pattern" when defining the URL of the route, and so it's going to look like some weird Drupalism that we use "pattern" there (even though I realize this was the old standard in Symfony 2.1 and below).

We could keep this backwards compatible by still allowing 'pattern', but I feel strongly we should switch the defaults in core anyway to match Symfony's docs to help eliminate newbie confusion. What say you?

Files: 
CommentFileSizeAuthor
#35 path-pattern-2051097-34.patch110.51 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,099 pass(es).
[ View ]
#31 drupal8.routing-system.2051097-31.patch121.49 KBneclimdul
PASSED: [[SimpleTest]]: [MySQL] 59,299 pass(es).
[ View ]
#31 interdiff.txt1.69 KBneclimdul
#29 pattern-path-2051097-29.patch120.82 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,138 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#29 interdiff.txt1.59 KBdawehner
#27 path-pattern-2051097-27.patch119.22 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,783 pass(es), 10 fail(s), and 23 exception(s).
[ View ]
#27 interdiff.txt2.39 KBdawehner
#25 path-pattern-2051097-25.patch117.49 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 2051097-pattern-to-path-15.patch86.65 KBtlattimore
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 simplytestme.txt9.08 KBManuel Garcia
#5 drupal-pattern-path-routing-2051097-5.patch86.92 KBManuel Garcia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-pattern-path-routing-2051097-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 drupal-pattern-path-routing-2051097-2.patch81.75 KBManuel Garcia
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 drupal-2051097-1.patch643 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,935 pass(es).
[ View ]

Comments

Issue tags:+Novice
StatusFileSize
new643 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,935 pass(es).
[ View ]

+1 especially because we ask people to look at the docs of symfony if they want to understand the route system.

Technically we don't even have to switch a single line as symfony itself is backport compatible as you said. Accessing the pattern/path is technically the same, so what about creating maybe the biggest task in amount of actual changes :)

Status:Active» Needs review
StatusFileSize
new81.75 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Executed from drupal root:
find ./ -name *.routing.yml -type f -exec sed -i 's/pattern:/path:/' {} \;

Patch includes the change from @dawehner on #1

Status:Needs review» Needs work

The last submitted patch, drupal-pattern-path-routing-2051097-2.patch, failed testing.

Tested the patch #2 on simpletest.me and it shows error is due to this :

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: http://s2d541bc5126fd0f.s3.simplytest.me/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
StatusText: OK
ResponseText:
Fatal error:  Call to a member function id() on a non-object in /home/s2d541bc5126fd0f/www/core/modules/menu/menu.module on line 200

StatusFileSize
new86.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-pattern-path-routing-2051097-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I'm not realy sure if this is what we are supposed to be doing besides the changes on my previous patch, but here it is...

I've renamed getPatterns() method into getPath(), and updated every call that I saw in Drupal core files:

  • core/modules/system/lib/Drupal/system/Tests/Routing/RouteProviderTest.php
  • core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php
  • core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php (commented line)
  • core/lib/Drupal/Core/Routing/MatcherDumper.php
  • core/lib/Drupal/Core/Routing/CompiledRoute.php (where it's defined)

This patch also includes the changes from #2.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-pattern-path-routing-2051097-5.patch, failed testing.

Here is the error log from simplytest.me

The interesting part I assume is this:
SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'pattern' cannot be null: INSERT INTO {router} (name, route_set, fit, pattern, pattern_outline, number_parts, route) VALUES

I've looked into core/lib/Drupal/Core/Routing/MatcherDumper.php, and inside public function dump we are using getPath() to get the value to be inserted in the DB... So it looks ok to me. Any clues anyone?

This also raises the question about the table field 'pattern', should we rename it also?

Forgot to attach.

Invalid file extension error page got rid of tags apparently...

Issue tags:+Stalking Crell

I think crell the maintainer of Routing system will know better.So Adding tags.

Status:Needs review» Needs work
Issue tags:+DX (Developer Experience), +Novice, +API change, +Backwards compatible API change, +Stalking Crell

The last submitted patch, drupal-pattern-path-routing-2051097-5.patch, failed testing.

Issue tags:+Needs reroll

Looks like this patch has gotten stale so tagging.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new86.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here's a re-roll of the patch in #5. No interdiff of course because of the conflict.

Status:Needs review» Needs work

The last submitted patch, 2051097-pattern-to-path-15.patch, failed testing.

@@ -139,8 +139,8 @@ public function getRoute() {
-  public function getPattern() {
-    return $this->route->getPattern();
+  public function getPath() {
+    return $this->route->getPath();
   }

It would be better to keep the old method, so there is no api change at all. getPattern could get a @deprecated tag so there is no API change needed.

Issue tags:+Approved API change

This is probably at least major. catch says:

"major or critical, feels like an 'upgrade to latest symfony and don't use deprecated crap' issue."

so tagging.

Priority:Normal» Major

Well, we won't update to symfony > 2.3 anyway so at least for the 8.x release cycle we are totally safe.

The last submitted patch, 2051097-pattern-to-path-15.patch, failed testing.

Actually catch has indicated he wants to track Symfony 2.4, 2.5, etc., since they're going to be very careful with BC for the rest of the 2.x cycle.

I am fine with this change. There's a couple of tweaks that have been made to the route yaml file since we first started using it (all BC-able), which we should probably adopt. This is the big one.

I think it's still safe to rename the DB fields, in which case we should rename both pattern and pattern_outline.

And let's make sure that our code isn't bypassing whatever it is that supports both pattern and path in Symfony, since we do bypass/override various pieces of it.

Yes I'd like to track 2.4, 2.5 if it's at all possible. Same goes for other PHP libraries we've pulled in as well as JavaScript.

I'm concerned about us eventually getting locked out of performance improvements and etc. because they're no longer backported to 2.3 (even if that's still LTS and has security support).

How realistic that is we won't know until we get there.

Status:Needs work» Needs review
StatusFileSize
new117.49 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerole

Status:Needs review» Needs work

The last submitted patch, path-pattern-2051097-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.39 KB
new119.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,783 pass(es), 10 fail(s), and 23 exception(s).
[ View ]

Ha, a few places tried to hide.

Status:Needs review» Needs work

The last submitted patch, path-pattern-2051097-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.59 KB
new120.82 KB
FAILED: [[SimpleTest]]: [MySQL] 59,138 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

This should be it.

Status:Needs review» Needs work

The last submitted patch, pattern-path-2051097-29.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
new121.49 KB
PASSED: [[SimpleTest]]: [MySQL] 59,299 pass(es).
[ View ]

quick test fixes.

Status:Needs review» Reviewed & tested by the community

> we ask people to look at the docs of symfony if they want to understand the route system.

LOL

Title:Change "pattern" to "path" in *.routing.yml filesChange notice: Change "pattern" to "path" in *.routing.yml files
Assigned:Unassigned» webchick
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record, +pants

Committed and pushed to 8.x. Thanks!

Needs a change notice. I'll take care of it.

Also, I don't get what's so funny in #32. I don't understand why we'd duplicate all of the documentation at http://symfony.com/doc/current/book/routing.html.

Title:Change notice: Change "pattern" to "path" in *.routing.yml filesChange "pattern" to "path" in *.routing.yml files
Assigned:webchick» Unassigned
Status:Active» Needs work
Issue tags:-Needs change record

Done: https://drupal.org/node/2089727 and about 50 other change notices updated, too. :P

Tim noticed a bug with spaces in the patch so marking back to needs work.

Title:Change "pattern" to "path" in *.routing.yml filesChange notice: Change "pattern" to "path" in *.routing.yml files
Assigned:Unassigned» webchick
Status:Needs work» Needs review
Issue tags:+Needs change record
StatusFileSize
new110.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,099 pass(es).
[ View ]

This has an extra space everywhere. Quick fix.

Status:Needs review» Reviewed & tested by the community

Title:Change notice: Change "pattern" to "path" in *.routing.yml filesChange "pattern" to "path" in *.routing.yml files
Status:Reviewed & tested by the community» Fixed
Issue tags:-Needs change record

Thanks. /me slaps self.

Committed and pushed to 8.x.

/me slaps self twice

It seems this change was in fact not backwards-compatible. I get

Undefined index: path in Drupal\Core\Routing\RouteBuilder->rebuild()

in Libraries API in a previously passing HEAD. I don't suggest reverting this or anything just noting here, for reference.

Shoot, yes, you are right. API freeze got a lot slushier since the OP was written. :)

We should probably write a routing-specific YAML parser that just rips code from the Symfony one. It handles both versions as well as a number of other recent improvements. (We can't use Symfony's class directly because it depends on the Symfony Config component; bah.)

Automatically closed -- issue fixed for 2 weeks with no activity.