Working on contrib Tour UI running into #2051097: Change "pattern" to "path" in *.routing.yml files I checked for the pattern used by the routing files.

$ find . -name "*.routing.yml" -exec cat {} \; | grep '  path' | cut -c -10 | sort -u
  path: "/
  path: '/
  path: 'a
  path: 'c
  path: 'm
  path: 't
  path: 'u
  path: aj
  path: bl

So we use different quotes or no quote and either use a starting '/' or not.

What should it be?

no quoting

$ find . -name "*.routing.yml" -exec grep -l "  path\: [^\'"]" {} \;
./modules/block/custom_block/custom_block.routing.yml
./modules/system/tests/modules/ajax_test/ajax_test.routing.yml

Double quote

$ find . -name "*.routing.yml" -exec grep -l '  path: "' {} \;
./modules/system/tests/modules/test_page_test/test_page_test.routing.yml

No starting /

Using

$ find . -name "*.routing.yml" -exec grep  "  path: '" {} \; | cut -c -10 | sort -u

then

$ find . -name "*.routing.yml" -exec grep  "  path: '[abmtu]" {} \;
  path: 'admin/config/system/actions/configure/{action}/delete'
  path: 'admin/structure/config_test/add'
  path: 'admin/structure/config_test/manage/{config_test}'
  path: 'admin/structure/config_test/manage/{config_test}/enable'
  path: 'admin/structure/config_test/manage/{config_test}/disable'
  path: 'admin/structure/config_test/manage/{config_test}/delete'
  path: 'admin/structure/contact/manage/{contact_category}/delete'
  path: 'admin/config/regional/content_translation/translatable/{entity_type}/{field_name}'
  path: 'admin/reports/event/{event_id}'
  path: 'admin/reports/fields'
  path: 'admin/help'
  path: 'admin/help/{name}'
  path: 'admin/config/media/image-styles/manage/{image_style}/delete'
  path: 'admin/config/media/image-styles/manage/{image_style}/effects/{image_effect}/delete'
  path: 'admin/config/regional/language/detection/browser/delete/{browser_langcode}'
  path: 'admin/structure/menu/item/{menu_link}/reset'
  path: 'admin/structure/menu/item/{menu_link}/delete'
  path: 'admin/structure/menu/manage/{menu}/delete'
  path: 'admin/config/search/path/delete/{pid}'
  path: 'user/{user}/shortcuts'
  path: 'admin/config/regional/date-time/formats/manage/{date_format}/delete'
  path: 'admin/config/regional/date-time/locale/{langcode}/reset'
  path: 'admin/modules'
  path: 'admin/modules/list/confirm'
  path: 'admin/index'
  path: 'admin/modules/uninstall'
  path: 'admin/modules/uninstall/confirm'
  path: 'menu_callback_description'
  path: 'url-alter-test/foo'
  path: 'admin/structure/taxonomy/manage/{taxonomy_vocabulary}'
  path: 'tour-test-1'
  path: 'tour-test-1/action'
  path: 'tour-test-2/subpath'
Files: 
CommentFileSizeAuthor
#29 drupal8.routing-system.2095121-29.patch18.01 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,698 pass(es).
[ View ]
#27 drupal8.forms-system.2071115-1.patch6.25 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]
#23 drupal8.routing-system.2095121-23.patch16.89 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 59,104 pass(es).
[ View ]
#19 drupal8.routing-system.2095121-19.patch16.81 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 58,712 pass(es).
[ View ]
#18 drupal8.routing-system.2095121-17.patch0 bytesclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 59,045 pass(es).
[ View ]
#13 drupal8.routing-system.2095121-13.patch16.49 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 58,714 pass(es).
[ View ]
#8 drupal8.routing-system.2095121-7.patch111.76 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 58,854 pass(es).
[ View ]
#1 drupal8.routing-system.2095121-1.patch16.49 KBclemens.tolboom
FAILED: [[SimpleTest]]: [MySQL] 58,963 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

clemens.tolboom’s picture

Assigned:clemens.tolboom» Unassigned
Status:Active» Needs review
StatusFileSize
new16.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,963 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
jibran’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Quick fix

Fair enough. Thanks @clemens.tolboom

joachim’s picture

Status:Reviewed & tested by the community» Needs review
Issue tags:-Quick fix

The docs state that the initial '/' is needed, but if in fact we can dispense with it, we definitely should standardize on not using it.

D7 doesn't use an initial '/ in hook_menu(), so it would be one less change to learn for developers transitioning from D7.

clemens.tolboom’s picture

@joachim what docs?

I was about to say the l() requires a start '/' (as I thought in the past) but there is only one left in core.

$ cd core/modules
$ grep -r " l('/" * | grep '/'
views_ui/lib/Drupal/views_ui/ViewListController.php:          $all_paths[] = l('/' . $path, $path);

It would be great to have both hook_menu() and the routing inline.

joachim’s picture

The change record here https://drupal.org/node/1800686 says:

Each route consists of a path e.g. /admin/content/book, including preceding slash. The maximum of nine parts remains in place.

Examples module, presumably based on this, says this as well.

joachim’s picture

Sorry, didn't mean to remove the tag.

Though should we file a separate issue for the initial slash thing, as that probably isn't a quick fix -- we'd need input from routing system developers to know whether it's expected behaviour or a glitch that it works without the '/', and then docs would need updating etc etc.

Status:Needs review» Needs work

The last submitted patch, drupal8.routing-system.2095121-1.patch, failed testing.

clemens.tolboom’s picture

Status:Needs work» Needs review
StatusFileSize
new111.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,854 pass(es).
[ View ]

Attached patch has the initial '/' removed.

$ find . -name "*.routing.yml" -exec sed -i -e "s/  path: '\//  path: '/" {} \;
clemens.tolboom’s picture

I cannot reproduce on local the failures from #1

The test block appears in the custom category. Other DisplayBlockTest.php 102 Drupal\block\Tests\Views\DisplayBlockTest->testBlockCategory()
The second cloned test block appears in the custom category. Other DisplayBlockTest.php 120 Drupal\block\Tests\Views\DisplayBlockTest->testBlockCategory()

So let's see what #8 does.

joachim’s picture

Issue tags:+Quick fix

Oops.

clemens.tolboom’s picture

A side note about the 'page not found' logic display path including '/' like

The requested page "/taxonomy/term/1" could not be found.
tim.plunkett’s picture

This is o

+++ b/core/modules/system/system.routing.yml
@@ -306,14 +306,14 @@ system.theme_set_default:
'<front>':
-  path: '/'
+  path: ''

This is wrong, AFAIK. We need to *add* all of the slashes, not remove them.

clemens.tolboom’s picture

Assigned:Unassigned» clemens.tolboom
StatusFileSize
new16.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,714 pass(es).
[ View ]

@tim.plunkett then we can use #1.

Here is a reroll to make testbot happy again.

clemens.tolboom’s picture

Assigned:clemens.tolboom» Unassigned

No need to be assigned.

jibran’s picture

Category:bug» task
Status:Needs review» Needs work
Issue tags:-Quick fix

After applying the patch and runnig this

$ find . -name "*.routing.yml" -exec cat {} \; | grep '  path' | cut -c -10 | sort -u
  path: "/
  path: '/

I still have some routes using " can we fix also fix these.

find . -name "*.routing.yml" -exec grep -l '  path: "' {} \;
./modules/system/tests/modules/test_page_test/test_page_test.routing.yml

IMHO it is not a bug it is simply a task.

joachim’s picture

> This is wrong, AFAIK. We need to *add* all of the slashes, not remove them.

But they work without slashes, don't they? Is that unexpected behaviour?

If it works either way, why not standardize on the form that we used in D7?

longwave’s picture

Title:To many flavours for path: pattern in routing.yml files.» Too many flavours for path: pattern in routing.yml files.

I keep mis-parsing the title of this issue :)

clemens.tolboom’s picture

Title:Too many flavours for path: pattern in routing.yml files.» To many flavours for path: pattern in routing.yml files.
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,045 pass(es).
[ View ]

Not sure what went wrong :-/

clemens.tolboom’s picture

Status:Needs work» Needs review
StatusFileSize
new16.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,712 pass(es).
[ View ]

(sigh)

jibran’s picture

Title:To many flavours for path: pattern in routing.yml files.» Too many flavours for path: pattern in routing.yml files
Status:Needs review» Reviewed & tested by the community

RTBC if green

tim.plunkett’s picture

Title:Too many flavours for path: pattern in routing.yml files» Ensure every path in routing.yml files has a leading slash
Issue tags:+Quick fix

+1

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

clemens.tolboom’s picture

Status:Needs work» Needs review
StatusFileSize
new16.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,104 pass(es).
[ View ]

Pseudo diff between #19

-  path: 'comment/reply/{node}/{pid}'       | -  path: 'comment/reply/{entity_type}/{entity_id}/{field_name
+  path: '/comment/reply/{node}/{pid}'       | +  path: '/comment/reply/{entity_type}/{entity_id}/{field_nam
jibran’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

mcrittenden’s picture

Issue tags:-Needs reroll

Tags

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Sorry, once again no longer applies. :(

jibran’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new6.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]

Yay!!

find . -name "*.routing.yml" -exec cat {} \; | grep '  path' | cut -c -10 | sort -u
  path: '/
clemens.tolboom’s picture

Status:Needs review» Needs work

This doesn't look like the correct patch.

jibran’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new18.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,698 pass(es).
[ View ]

arghh

clemens.tolboom’s picture

Status:Needs work» Reviewed & tested by the community

Assuming the testbot does not fail this is RTBC

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committing it while it's hot!

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary:View changes

find command finds double quotes.