Selecting "Only the listed pages" when positioning a block doesn't give any feedback when an invalid path is entered.

Title doesn't fully describe bug, but I can guess you're talking about block visibility settings, specifically when setting block to show on listed pages. Is that right?

1) Doesn't have to be javascript, can be a simple validation on PHP
2) Browse doesn't sound good, we have router items, i.e node/[nid] wich can hold millions of entries. They are also not easy to pre-determine. As for interface, auto-complete sounds better.

Can you please post the exact steps to replicate this issue.

@benjy

This is simply a usability improvement, not a bug to reproduce.

When you restrict block visibility to a page and type the wrong path, nothing happens (as expected), but it's hard to figure why. I suggested a simple validation of paths entered as a mean to help. Could be a warning like: "The path you entered does not seem to exist, are you sure it's right?"

@franz, thanks I understand now. It's a good point, even if it is a bit of an edge case.

I've updated the title to be a little clearer.

Attached patch adds this usability improvement.

If an invalid path is entered it will set a warning message.

@@ -253,6 +254,18 @@ public function validate(array $form, array &$form_state) {
+      $internal_path = Drupal::service('path.alias_manager')->getSystemPath(str_replace(array("\r", "\n"), '', $path));

This needs injecting as a dependency. @see \Drupal\system\Form\SiteInformationForm for an example.

Attached patch uses dependency injection

Can you please add a property to the class for $aliasManager

@@ -7,6 +7,7 @@
+use Drupal;

This is not needed but we do need a use Drupal\Core\Path\AliasManagerInterface;

@@ -44,11 +45,12 @@ class BlockFormController extends EntityFormController implements EntityControll
+  public function __construct(ModuleHandlerInterface $module_handler, EntityManager $entity_manager, QueryFactory $entity_query_factory, AliasManagerInterface $alias_manager) {

We also need to update the doc comment.

Also changed everything commented in #8

I guess the test needs to be updated to reflect the new constructor.

@@ -43,12 +51,15 @@ class BlockFormController extends EntityFormController implements EntityControll
+   *   The entity query factory.

This looks to have been copy and pasted by accident.

Try and add the alias manager to the test. Drupal\Tests\Core\PathProcessor\PathProcessorTest has an example of using the AliasManager in a test.

Updates in attached patch:
From #14:
- Fixed copy/paste error
- Added alias manager to the test
From a discussion with dawehner in #drupal-contrib
- Changed $container->get('path.alias_manager') to $container->get('path.alias_manager.chached')

I'm not sure what the difference is between $container->get('path.alias_manager') and $container->get('path.alias_manager.chached') but maybe someone can explain?

Either way, you've missed spelt "cached".

OK, as explained by @tim.plunkett to me on IRC. The non-cached version of alias_manager recalculates paths when you call a method. We should in most cases be using the cached version.

Fixed the typo

The patch in #18 works as advertised. I wonder though about the implementation. I think that this validation should probably happen on the edit form and not after the submit. I understand why it was implemented the way it was, but getting an error (or even a warning, in this case) after you've navigated away from a page seems like it may not be the most user-friendly workflow.

Settings the status to RTBC since the patch does work as described.

Great effort! Please keep it up!

Luke

Edit: I think that the warning message should include a link to the block (in the appropriate region) that was just edited as well as the alias that triggered the warning. That would avoid the odd work-flow as best as it can be.

Sorry for the quick update -- this patch does not handle wildcards. Changing this back to "Needs Work" until that can be reliably addressed as well.

Thanks for the review lukewertz.

I'll fix the wildcard handling and I'll look into a way to do a javascript check on top of this implementation.
I'm not sure about what you exactly mean with:

I think that the warning message should include a link to the block (in the appropriate region) that was just edited as well as the alias that triggered the warning.

Would that be a link to an anchor? I foresee situations where the block is invisible on the page where the error message is shown. An anchor wouldnt' do any good then. Or do you mean a link back to the block configuration page?

I've moved the path validation to a new function and added wildcard handling, This way i can reuse the code when i'm starting work on the javascript check.
I'm unable to do an access check on dynamic system paths like node/% because of #876580: drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus) and i'm still thinking about a way to implement an access check for the records in the url_alias table.

My current to do list:

• Add a link to block configuration page in the warning message.
• Add Javascript to check before form submission,
• Add tests for the path validation function.

1. I'm not sure JavaScript is correct here. Why can't we just use form_set_error in the validate function when there is an invalid path?

2. The last patch has a logic error in validate_paths() and doesn't work.

3. The block comment on validate_paths() needs fixing up.

4. I'm not sure about a function called validate_paths() which returns an array of error paths otherwise an empty array. Could we make it a little clearer? Maybe a validate path method that just returns true or false?

Good work so far!

I don't think we should do the validation because (in this instance), I think people should be allowed to submit invalid URLs if they want to. We shouldn't stop them as site builders from entering a path that they know (or think) will be valid even if it isn't currently.

I don't think the JS implementation is necessary.

If we add more context to the warning message after save (specifically: a link to the block in the correct context that was just edited), that should be enough.

Think of this workflow: you're working on a site with 150 blocks (including a block that is in multiple regions). You've just saved a block that has an invalid path. If the warning only displays the block name (as well as the invalid path) that you just edited, you might forget what instance of the block you were editing. I think simply providing more information in the warning would suffice. That way the warning could be dismissed without blocking the save.

Thanks,
Luke

1. I'm not sure JavaScript is correct here. Why can't we just use form_set_error in the validate function when there is an invalid path?

I agree with lukewertz's argument in #25 on not using form_set_error. Just informing the user about an invalid path should suffice. I think the current implementation using drupal_set_message does an excellent job of exactly that. However I do think checking paths before submitting the form could be a UX improvement since users would be informed about the incorrect path in time to alter it. This should happen in an unobtrusive way, which I'm still thinking about.
Perhaps we could make that a follow up issue, since i'm not that experienced with (Drupal) javascript.

2. The last patch has a logic error in validate_paths() and doesn't work.

Strange, i've tested it with a combination of existing and non existing paths and it worked like a charm locally. Any problem you've had will probably be fixed in the attached patch.

4. I'm not sure about a function called validate_paths() which returns an array of error paths otherwise an empty array. Could we make it a little clearer? Maybe a validate path method that just returns true or false?

I agree and have rewritten the code (and comments) accordingly, improving readability in the process.

#26: core-block-invalid-path-feedback-1164722-26.patch queued for re-testing.

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -240,6 +252,36 @@ protected function actions(array $form, array &$form_state) {
   +   * Checks a string to see if it is a valid path.
   +   * @param string $path
   +   * Path to check.
   +   *
   +   * @return boolean
   +   * TRUE if the path is valid, FALSE if it is not.
   +   *
   +   * TODO rewrite to do an access check on dynamic paths once issue 876580 has been fixed.
   

   This docblock is formatted incorrectly.

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -253,6 +295,19 @@ public function validate(array $form, array &$form_state) {
   +      dpm($form);
   

   :)

3. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -253,6 +295,19 @@ public function validate(array $form, array &$form_state) {
   +        drupal_set_message(t("It seems you've set the <a href=\"@url\">block</a> to be @visibility on an invalid path. Are you sure <b>@path</b> exists and that you have access to it?", $substitutes), 'warning');
   

   This should be form_set_error, not drupal_set_message. Also, it doesn't "seem" to be invalid, it is invalid. This should be rewritten more like an error message.

Also, we should test the new validation.

Can this please NOT be done? It is not uncommon for people to create blocks ahead of time, knowing what they will set up the path to be in the future, and enter those paths in the "only show on these pages" section. To force the workflow to make the user create the page with the path first is undesirable...IMHO anyway...if the path doesn't exist, that entry does nothing...what's the harm?

Full disclosure, i haven't seen what, if anything, else has changed in D8 for block workflow, i'm just speaking generally. will set up a D8 instance soon to start pitching in, promise!

I agree with scotself. Displaying a warning after saving the configuration will be helpful for people who unknowingly entered a wrong path , while still allowing people who like to create blocks ahead of time to knowingly configure the block to show on pages that don't exists (yet).

I've changed the docblock formatting and warning message. I've also removed the call to dpm. I'll hold off on changing to form_set_error untill we've reached a consensus and I will be adding tests for the validation.

I've updated the patch with test for the new validation function. Besides that I've also had a discussion with dawehner in #drupal-contrib about the use of drupal_valid_path in the valid_path function. Since drupal_valid_path doesn't work for dynamic paths and still uses the old routing system in stead of the new I will rewrite the valid_path function to make use of the new routing system.

#33: core-block-invalid-path-feedback-1164722-33.patch queued for re-testing.

This looks good!

I've updated the valid_path function to also use the new routing system. As a result the aliasManager i've added before was no longer needed, so i removed it.

Missed a comma

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -249,6 +250,80 @@ protected function actions(array $form, array &$form_state) {
   +  private function valid_path($path) {
   

   Let's try to never make functions private as they are harder to reuse.

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -249,6 +250,80 @@ protected function actions(array $form, array &$form_state) {
   +    if($routes->count() > 0){
   +      if(strpos($processed_path, '%')){
   ...
   +      else{
   ...
   +    else{
   ...
   +      if(!strpos($processed_path, '%')) {
   

   Some minor codestyle issues: there should be a space after the else and the if.

3. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -249,6 +250,80 @@ protected function actions(array $form, array &$form_state) {
   +      // No match was found in the new router system.
   ...
   +        // If the path isn't dynamic we can just check it using drupal_valid_path.
   +        return drupal_valid_path($processed_path);
   +      }
   

   There should be a @todo that we won't support it anymore once the old router system is removed.

I've addressed the code style issues, added the @todo and made valid_path() public.

#41: core-block-invalid-path-feedback-1164722-41.patch queued for re-testing.

When i run the failing test locally it just passes. Unfortunately I currently don't have much time, so if someone else could chip in that would be greatly appreciated.

I've located and fixed the bug in the test.
Because of the recent additions/changes to the routing systems I decided to also review the valid_path() function to ensure it still worked as designed. It didn't so i've also made some alterations there.

#46: core-block-invalid-path-feedback-1164722-46.patch queued for re-testing.

I applied the patch from #46 and get:

Page not found
The requested page "/admin/structure/block/manage/mycustomblock_2" could not be found.

.. if I enter any path into the field "Only the listed pages". It doesn't matter if the path exists or not. I assume something else has changed since the patch was written.

I've tried to reproduce the behaviour you mention, but i've been unable to do so. Could you provide a more detailed description of how to reproduce this bug or even better, a patch with a failing test.

During my attempts to reproduce the bug you've mentioned i've also uncovered a routing exception that occurs when the only 'path' I put in is a space. I'll look into that and will also incorporate it into the test.

There is the possibility, that I didn't apply the patch correctly, so it would be nice if somebody else could verify, if the patch is working or not, before I try to reproduce it. For me it simply failed yesterday evening after cloning the latest Drupal 8 version.

Sorry I don't have time to look into it right now. But will have a look later today, if nobody else responded.

Ok .. I did a git reset --hard on my Drupal installation and pulled in the latest changes.

Afterwards I applied the patch: git apply -v core-block-invalid-path-feedback-1164722-46.patch again.
Checking patch core/modules/block/lib/Drupal/block/BlockFormController.php...
Checking patch core/modules/block/lib/Drupal/block/Tests/BlockTest.php...
Applied patch core/modules/block/lib/Drupal/block/BlockFormController.php cleanly.
Applied patch core/modules/block/lib/Drupal/block/Tests/BlockTest.php cleanly.

It still breaks with the error message mentioned above if I enter any path in the path inclusion field. I'm doing a manual test. I don't know what else I could do. Still .. it's perfectly possible, that I did something wrong ..

More coding standards.

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,108 @@ protected function actions(array $form, array &$form_state) {
   +    // first remove any spaces from the path.
   

   capitalize the first word.

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,108 @@ protected function actions(array $form, array &$form_state) {
   +    // Also remove any prepending slashes because they would cause the path to be handled as absolute.
   

   Exceeding 80 chars - happens in a lot of places.

3. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,108 @@ protected function actions(array $form, array &$form_state) {
   +    /** @var $path_processor \Drupal\Core\PathProcessor\InboundPathProcessorInterface */
   

   I don't we do this in our coding standards. Wondering whether we can also inject the processor or not.

4. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,108 @@ protected function actions(array $form, array &$form_state) {
   +    if($routes->count() > 0) {
   

   needs a space between if and ( - happens in a lot of places.

5. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,108 @@ protected function actions(array $form, array &$form_state) {
   +            }
   

   two spaces to much

6. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -94,6 +95,40 @@ function testBlockVisibilityListedEmpty() {
   +      $this->assertNoText("Are you sure $path exists and that you have access to it?", "<em>$path</em> exists, no error message was shown");
   

   Should use format_string form the replacements

7. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
   @@ -94,6 +95,40 @@ function testBlockVisibilityListedEmpty() {
   +      $this->assertText("Are you sure $path exists and that you have access to it?", "<em>$path</em> doesn't exists, error message was shown");
   

   same here

Working on this during drupal mentoring.
http://drupalmentoring.org/task/6501

Applied coding standard changes mentioned by swentel in #53 to legolasbo's patch in #46.

Just posted my progress with this, but still needs work to address the third change request:

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -284,12 +285,108 @@ protected function actions(array $form, array &$form_state) {
+    /** @var $path_processor \Drupal\Core\PathProcessor\InboundPathProcessorInterface */

I don't we do this in our coding standards. Wondering whether we can also inject the processor or not.

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
+    $route_provider = \Drupal::service('router.route_provider');

I asked about the injections in IRC and chx says that there isn't a standard for injecting into form controllers.

At any rate I see 4 different services in use here and I'm not sure what the standards are on which should be injected and which shouldn't.

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +  /*
   

   Should be /**

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +   * TRUE if the path is valid, FALSE if it is not.
   

   The descriptions after @param or @return definitions are indented with two extra spaces.

3. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +   *
   

   Redundant empty line.

4. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +    // Also remove any prepending slashes because they would cause the path to be
   

   Exceeds the 80-character limit.

5. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +    /** @var $path_processor \Drupal\Core\PathProcessor\InboundPathProcessorInterface */
   

   This does not belong here, as it is not a class property.

6. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +    /** @var $route_provider \Drupal\Core\Routing\RouteProviderInterface */
   

   No class property.

7. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +        // Attempt to match this path to provide a fully built request to the access
   

   Exceeds the 80-character limit.

8. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +      // @TODO This should be removed when the old router system is removed.
   

   Lower case @todo.

9. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
   +      // Since drupal_valid_path() currently fails for dynamic paths we'll manually
   

   Exceeds the 80-character limit.

10. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
    +      // Since drupal_valid_path() currently fails for dynamic paths we'll manually
    

    No contractions ("we'll" -> "we will") and exceeds the 80-character limit.

11. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
    +        // Access can't be checked without parameters. We'll assume the user has access.
    

    No contractions ("we'll" -> "we will") and exceeds the 80-character limit.

12. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -284,12 +285,113 @@ protected function actions(array $form, array &$form_state) {
    +        $result = db_query("SELECT * FROM {menu_router} where path = :path", array(':path' => $processed_path))->fetchAssoc();
    

    Let's use the query builder here. Inject the database service into the form controller, and then to $this->database->select().

Fixed comments as pointed out by Xano. Adding the progress here as I'm still working on the twelfth item mentioned.

Fixed #12 as well. Uploading the patch here.

I'm reviewing this patch, if necessary I'll reroll.

The patch in #60 was rather old, so I've rerolled it (thanks @pfrenssen for mentoring me). Many changes were done in the files incl deletions and renames so this patch might fail in testing.

Changing Status to Needs Review. You need to change the Status to "Needs Review" to get a patch reviewed.

I'll be working on this today

Many tests were failing because of some function calls to non existing functions. These functions have been removed in D8, so I've replaced them with their replacements. There are more problems to fix but I'm posting my progress so far, to see what testbot thinks of it.

Attached patch should fix the remaining failing tests.

Let's try again... fixed another bug.

Aaaand... fixed another few test bugs. this should do it.

Using $form['#action'] here seems quite wrong.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -142,11 +148,108 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +          '@block' => \Drupal::l(t('block'), Url::fromUri('base:/'.$form['#action'])),
   

   This is a little weird, and does not work if Drupal is installed in a subdirectory...

2. +++ b/core/modules/block/src/BlockForm.php
   @@ -142,11 +148,108 @@ protected function actions(array $form, FormStateInterface $form_state) {
   +        drupal_set_message(t("You've set the @block to be @visibility on an invalid path. Are you sure <strong>@path</strong> exists and that you have access to it?", $substitutes), 'warning');
   

   The '@' replacement formatter is incorrect. '@block' is a link that contains a prepared HTML anchor tag which was already run through check_plain() so we can use the "!" formatter instead.

• Updated the patch according to #78
• Improved the warning message a little by using the block label as the link title when available.
• Updated the tests accordingly

+++ b/core/modules/block/src/BlockForm.php
@@ -142,11 +149,128 @@ protected function actions(array $form, FormStateInterface $form_state) {
+   * Checks a string to see if it is a valid path.
+   *
+   * @param string $path
+   *   Path to check.
+   *
+   * @return boolean
+   *   TRUE if the path is valid, FALSE if it is not.
+   */
+  public function valid_path($path) {
+
+    // First remove any spaces from the path.
+    // Also remove any prepending slashes because they would cause the path to
+    // be handled as absolute.
+    $path = ltrim(trim($path),'/');
+    // Replace * placeholders with % placeholders
+    $path = str_replace('*', '%', $path);
+
+    // Process the path to support URL alias.
+    $path_processor = \Drupal::service('path_processor_manager');
+    $request = Request::create($path);
+    $processed_path = $path_processor->processInbound($path, $request);
+
+    // First we try the new router system.
+    $route_provider = \Drupal::service('router.route_provider');
+    $routes = $route_provider->getRoutesByPattern('/' . $processed_path);
+    if ($routes->count() > 0) {
+      if (strpos($processed_path, '%')) {
+        // Access to dynamic paths can't be checked without parameters.
+        // We'll assume the user has access.
+        return TRUE;
+      }
+      else {
+        $check_request = Request::create("/$processed_path");
+        $check_request->attributes->set('_system_path', $processed_path);
+
+        // Attempt to match this path to provide a fully built request to the
+        // access checker.
+        try {
+          $check_request->attributes->add(\Drupal::service('router')->matchRequest($check_request));
+        }
+        catch (NotFoundHttpException $e) {
+          return FALSE;
+        }
+
+        foreach ($routes->all() as $route) {
+          if (\Drupal::service('access_manager')->check(RouteMatch::createFromRequest($check_request))) {
+            return TRUE;
+          }
+        }
+      }
+    }
+    else {
+      // No match was found in the new router system.
+      // @todo This should be removed when the old router system is removed.
+      if (!strpos($processed_path, '%')) {
+        // If the path isn't dynamic we can just check it using
+        // drupal_valid_path.
+        return \Drupal::service('path.validator')->isValid($processed_path);
+      }
+      // Since drupal_valid_path() currently fails for dynamic paths we will
+      // manually check if these paths exist.
+      else {
+        // If no match has been found by now, we are probably dealing with an
+        // invalid path.
+        // It could however be a partial URL alias. As a last resort we will
+        // look for possible matches in the url_alias table.
+        $query = \Drupal::database()->select('url_alias');
+        $query->fields('url_alias', array('pid', 'source'));
+        $query->condition('alias', $path, 'LIKE');
+        $result = $query->execute()->fetchAllAssoc('pid');
+        foreach ($result as $alias_info) {
+          // Check each system path for access
+          if (\Drupal::service('path.validator')->isValid($alias_info->source)) {
+            return TRUE;
+          }
+        }
+      }
+    }
+
+    return FALSE;
+  }

We should check to see if this function can be replaced or simplified by using \Drupal::pathValidator()->isValid($path)

$ git apply block-invalid_path_feedback-1164722-79.patch 
error: patch failed: core/modules/block/src/BlockForm.php:7
error: core/modules/block/src/BlockForm.php: patch does not apply
error: patch failed: core/modules/block/src/Tests/BlockTest.php:7
error: core/modules/block/src/Tests/BlockTest.php: patch does not apply

How to make a re-roll: https://www.drupal.org/patch/reroll

Patch rerolled.

I am removing the Novice tag from this issue because it was applied in 2013 and there has been quite a bit of work done since then.

Also this issue is getting close to 100 comments which is a lot for a new contributor.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

Maybe we could just add a minimal validation to make sure it has a leading slash at least for 8?