Path alias UI allows node/1 and /node/1 as system path then fatals

Just a couple of random changes here and there so far.

Let's see

This is very much leftover technical debt from the menu link / path work around the Princeton sprint in January. Adding to the meta. #2430593: Store {url_alias}.source and .alias with a leading / is particularly relevant.

Good point yeah I'm currently doing #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals

Some more fixes.

Some more ...

Hopefully less ...

A couple of less ...

lesss ...

Needs issue summary update, please :)

From what I get form the patch this is forcing '/' for all paths?

Is this overall not the same problem space as link module with base:// and external://, etc. for user entered paths?

Would move path aliases to entities (issue exists in good shape) and re-using the menu link plugins make sense here (while keeping the same storage in the background)?

Would move path aliases to entities (issue exists in good shape) and re-using the menu link plugins make sense here (while keeping the same storage in the background)?

Well, the minority of the patch is the path UI, so this really does not help, its the fact that also path processing should use a starting slash, which is related to the path alias processor.
And then things get big. Entities are just a different storage layer, kinda an internal implementation detail, to be honest.

@effulgentsia agreed that we should do that as part of the patch.

Explained a bit more in the issue summary, still not perfect.

Patch did not applied anymore due to #2480811: Cache incoming path processing and route matching but well, let's fix the failing tests

Fixed the failures ... as well as added the update path, now we just need to fill out the tests ;)

Quick fix

Well, I totally think that the patch is not the most minimal patch you can write, but on the other hand, we should solve things the right way and not just implement local workarounds without thinking about the big picture.

Shows what we run into when we don't fix things all the way in the first place.

Kinda reminds me of history.

+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
@@ -50,8 +50,8 @@ public function processInbound($path, Request $request) {
+    if ($path == '/<front>') {
+      $path = '/';
     }

Do the magic constants like now need a leading slash when used?

Do the magic constants like now need a leading slash when used?

Good question!

Well yeah its what happens if you call \Drupal::urlGenerator()->generateFromPath('<front>')

Can we not either subclass or submit an upstream patch to wherever $this->makeSubrequest() is coming from so that it handles the trimming for you? Developers are going to miss this, so while we might fix core, contrib will end up hitting the same issue.

Can we not either subclass or submit an upstream patch to wherever $this->makeSubrequest() is coming from so that it handles the trimming for you? Developers are going to miss this, so while we might fix core, contrib will end up hitting the same issue.

This is an internal method and well ... every path has now "/" in there, so I'd argue its more consistent to have /<front> there. I hope its okay if we don't overcomplicate things. This code path is not even the recommended one, to be honest.

here is the upgrade path issue: #2511962: Upgrade path for 2509300

Sorry. It was a cross post of sorts: didn't realize <12hrs before I tagged dawehner already fixed it (we talked about it on IRC, I knew I wanted to tag it but he was faster and I didn't realize). Daniel is awesome like that :)

Let's fix the remaining bits.

Sorry. It was a cross post of sorts: didn't realize <12hrs before I tagged dawehner already fixed it (we talked about it on IRC, I knew I wanted to tag it but he was faster and I didn't realize). Daniel is awesome like that :)

Cool! I had issues with explaining issues in the past :)

Forgot to remove the actual update function ... note: this is always reviewable ...

+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
@@ -50,8 +50,8 @@ public function processInbound($path, Request $request) {
-    if ($path == '<front>') {
-      $path = '';
+    if ($path == '/<front>') {
+      $path = '/';

+++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
@@ -161,7 +161,7 @@ public function build(RouteMatchInterface $route_match) {
       $links[] = Link::createFromRoute($this->t('Home'), '<front>');

As long as the module-author-facing part of this API is still

<front>

, then I don't mind it being

/<front>

internally.

As long as the module-author-facing part of this API is still

Yeah to be clear, the recommended way: Url::fromRoute('<front>') still works as expected.
For sure, we could special case <front>, but then you could ask yourself, why ...

Better question... If the leading / is always required... why does <front> need to exist? It exists to differentiate from the empty string, we don't need to differentiate from the empty string anymore because the front page is /.

/ is synonymous with <front>. Let's just use that and deprecate <front> entirely. (We can't remove it right now, but we can deprecate it.)

Edit: Writing about <front> in HTML is really annoying. :-P

+1

Thank you for some feedback!

Better question... If the leading / is always required... why does need to exist? It exists to differentiate from the empty string, we don't need to differentiate from the empty string anymore because the front page is /.

/ is synonymous with . Let's just use that and deprecate entirely. (We can't remove it right now, but we can deprecate it.)

Edit: Writing about in HTML is really annoying. :-P

This code we are talking about is for url outbound processing ... existed though since ever in Drupal and getting rid of it in general should be part of a different issue.

To explain that again, Url::fromRoute('')->toString() would not pass <front> to the path processor.
For an actual incoming request you also end up with passing in '/', this is only for the following codepath $url_generator->generateFromPath | $url_assember->assemble(),
its all about outbound processing

On the other hand there are still a hell lot of instances, for example:

$ grep "drupalGet('<front>')" . -rn | wc -l
38

What about cleaning this up as part of a follow up? Why do we want to get rid of <front> as part of this issue?

Why do we want to get rid of <front> as part of this issue?

Because #55 does get rid of it, and replaces it with /<front>, which is nonsensical. What if instead of exposing that nonsense to path processors, we changed generateFromPath() to look for <front> and internally call generateFromRoute('<front>') on that instead, thereby still allowing drupalGet('<front>') to continue working for now? Then in a followup, we can finish removing all traces of <front> as a path?

Note, however, that #61 is not worth doing if it means the difference between this patch landing before the beta is tagged vs. after, since it would be wonderful to not need to write a hook_update_N() for this.

Because #55 does get rid of it, and replaces it with /, which is nonsensical

Well, I agree its odd, but not always start the path and special case <front> in that sense would not be sensical either, IMHO.
On the other hand, we do that already in either places, to be fair.

1. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
   @@ -47,8 +50,8 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
   +   *   - source (string): The internal system path with a starting lash.
   

   /me bats lashes

2. +++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorFront.php
   @@ -50,8 +50,8 @@ public function processInbound($path, Request $request) {
   +    if ($path == '/<front>') {
   
   +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -140,14 +140,16 @@ public function getRouteCollectionForRequest(Request $request) {
   +      $path = $path == '/' ? $path : rtrim($request->getPathInfo(), '/');
   

   nitpick, should be ===, $path = 0 would evaluate true here - see http://3v4l.org/vf405

3. +++ b/core/modules/config/src/Tests/ConfigSingleImportExportTest.php
   @@ -27,7 +27,7 @@ class ConfigSingleImportExportTest extends WebTestBase {
   -  public function testImport() {
   +  public function tesImport() {
   
   @@ -134,14 +134,14 @@ public function testImportSimpleConfiguration() {
   -  public function testExport() {
   +  public function tesExport() {
   

   debugging needs fix

4. +++ b/core/modules/path/src/Form/PathFormBase.php
   @@ -151,8 +163,18 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      $form_state->setErrorByName('source', 'The source path has to start with a slash.');
   ...
   +      $form_state->setErrorByName('alias', 'The alias path has to start with a slash.');
   
   +++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
   @@ -95,6 +95,10 @@ public static function validateFormElement(array &$element, FormStateInterface $
   +      $form_state->setError($element, t('The alias needs to start with a slash.'));
   
   +++ b/core/modules/system/src/Form/SiteInformationForm.php
   @@ -155,13 +165,17 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      $form_state->setErrorByName('site_frontpage', $this->t("The path '%path' has to start with a /.", ['%path' => $form_state->getValue('site_frontpage')]));
   
   @@ -172,6 +186,12 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      $form_state->setErrorByName('site_403', $this->t("The path '%path' has to start with a /.", ['%path' => $form_state->getValue('site_403')]));
   ...
   +      $form_state->setErrorByName('site_404', $this->t("The path '%path' has to start with a /.", ['%path' => $form_state->getValue('site_404')]));
   

   Not seeing any tests for this new validation?

Thank you larowlan!

Ups.

I've done a superficial review and nothing is jumping out at me.

RTBC + 1

Makes sense and it is good to see /foo in code everywhere :).

Discussed some of the rtrimming in RouteProvider with @dawehner in IRC - I think we might be doing one too many but there is no harm as far as I can see.

Committed e044adb and pushed to 8.0.x. Thanks!

Great to see this land. I started reviewing this just prior to that, so here's what I found before getting the notification that it landed, but this is incomplete.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
   @@ -66,7 +66,7 @@ protected static function getPriority() {
   -    $this->makeSubrequest($event, $path, Response::HTTP_FORBIDDEN);
   +    $this->makeSubrequest($event, trim($path, '/'), Response::HTTP_FORBIDDEN);
   

   Do we have a followup for makeSubrequest() to expect $path to have a leading slash?

2. +++ b/core/lib/Drupal/Core/Path/AliasWhitelist.php
   @@ -107,7 +107,7 @@ public function get($offset) {
   -    $exists = $this->aliasStorage->pathHasMatchingAlias($root);
   +    $exists = $this->aliasStorage->pathHasMatchingAlias('/' . $root);
   

   Do we have a follow-up for AliasWhitelist to expect $root to have a leading slash?

3. +++ b/core/lib/Drupal/Core/Path/PathMatcher.php
   @@ -101,7 +101,7 @@ public function isFrontPage() {
   -        $this->isCurrentFrontPage = ($url->getRouteName() && $url->getInternalPath() === $this->getFrontPagePath());
   +        $this->isCurrentFrontPage = ($url->getRouteName() && '/' . $url->getInternalPath() === $this->getFrontPagePath());
   

   getInternalPath() is marked as deprecated. Will we remove it before 8.0.0? If not, do we want to change its return value to have a leading slash?

4. +++ b/core/lib/Drupal/Core/Path/PathValidator.php
   @@ -155,10 +156,10 @@ protected function getPathAttributes($path, Request $request, $access_check) {
   -    $path = $this->pathProcessor->processInbound($path, $request);
   +    $path = $this->pathProcessor->processInbound('/' . $path, $request);
   

   Do we have a followup for PathValidator to expect $path to have a leading slash?

Good points yeah I tried to restrict the changes to a reasonable level.

Here is a follow up: #2514166: Use starting slash in :: makeSubrequest AliasWhitelist and PathValidator

Atleast site configuration page is broken after this going in. Created follow-up for that: #2514196: AliasManager->getAliasByPath() handles $path in confusing manner.

Again, is it too much to ask for a change record when you commit a change that you know will break existing contribs? Just write up the changes you put in, how long does that take? A hell of a lot less time than it took me to track down why my tests started failing ...

So the solution to $simpletest->drupalGet('/'); throwing a 'path needs a leading slash' exception is $simpletest->drupalGet(''); ??

So the solution to $simpletest->drupalGet('/'); throwing a 'path needs a leading slash' exception is $simpletest->drupalGet(''); ??

Feel free to open up an issue for that as well

Working on a CR now.

Filed #2516806: After path requirement change, drupalGet('/') must be replaced with drupalGet('')

Again, is it too much to ask for a change record when you commit a change that you know will break existing contribs? Just write up the changes you put in, how long does that take? A hell of a lot less time than it took me to track down why my tests started failing ...

Yes I simply forgot about creating a change record, so please remind yourself that there are humans on the other side of the internet.

Created the change record.

Is there anything else we need to close this down again?

Published the CR, back to fixed.

Added #2528414: Block visibility by path docs are missing leading slash.