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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: +Novice
FileSize
643 bytes

+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 :)

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
81.75 KB

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.

naveenvalecha’s picture

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
Manuel Garcia’s picture

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.

Manuel Garcia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Manuel Garcia’s picture

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?

Manuel Garcia’s picture

Forgot to attach.

Manuel Garcia’s picture

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

naveenvalecha’s picture

Issue tags: +Stalking Crell

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

dawehner’s picture

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

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.

tlattimore’s picture

Issue tags: +Needs reroll

Looks like this patch has gotten stale so tagging.

tlattimore’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
86.65 KB

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.

dawehner’s picture

@@ -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.

webchick’s picture

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.

webchick’s picture

Priority: Normal » Major
dawehner’s picture

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

amontero’s picture

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

Crell’s picture

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.

catch’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
117.49 KB

Rerole

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
119.22 KB

Ha, a few places tried to hide.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
120.82 KB

This should be it.

Status: Needs review » Needs work

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

neclimdul’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
121.49 KB

quick test fixes.

chx’s picture

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

webchick’s picture

Title: Change "pattern" to "path" in *.routing.yml files » Change 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.

webchick’s picture

Title: Change notice: Change "pattern" to "path" in *.routing.yml files » Change "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.

tim.plunkett’s picture

Title: Change "pattern" to "path" in *.routing.yml files » Change notice: Change "pattern" to "path" in *.routing.yml files
Assigned: Unassigned » webchick
Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
110.51 KB

This has an extra space everywhere. Quick fix.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Title: Change notice: Change "pattern" to "path" in *.routing.yml files » Change "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.

dawehner’s picture

/me slaps self twice

tstoeckler’s picture

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.

webchick’s picture

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

Crell’s picture

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.