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'
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

Assigned: clemens.tolboom » Unassigned
Status: Active » Needs review
FileSize
16.49 KB
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
FileSize
111.76 KB

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
FileSize
16.49 KB

@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.
FileSize
0 bytes

Not sure what went wrong :-/

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
16.81 KB

(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
FileSize
16.89 KB

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
FileSize
6.25 KB

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
FileSize
18.01 KB

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.

chintukarthi’s picture

Hi, I am having a path as below,
path : /drupal/add/new?row=1

Where the number is dynamic.

like,
path : /drupal/add/new?row=2,
path : /drupal/add/new?row=3, etc,

I need to give this in routing file. How to give this one?

I tried as below,

path: /drupal/add/new?row={id},

but it isn't woking. Any ideas?

Thanks.