Block page visibility paths are not validated, using path a without leading forward slash does not work

I have tested the patch and its working fine in drupal 8.2.

I have tested the patch with drupal 8.2 and properly validating the forward slash on the url key.

Nice work!
This needs automated tests before it can go in

I am working on this issue.

<?php

namespace Drupal\Tests\block\Functional;

//namespace Drupal\simpletest;

use \Drupal\Tests\BrowserTestBase;

use \Behat\Mink\WebAssert;
/**
* Tests that the blocked page's visibility paths are validated, using a path without leading forward slash works
*
* @group pages
*/
class ForwardSlashTest extends BrowserTestBase{
protected function setUp() {

parent::setUp();

$this->drupalLogin($this->rootUser);

}

public function testForwardSlash(){

$this->drupalGet('/admin/structure/block/manage/bartik_branding');

$this->submitForm(['visibility[request_path][pages' => 'user/login'], 'Show for the listed pages');

$this->assertText('Paths require a leading forward slash when used with the Pages setting.');

}

public function checkRenderingTest(){

$this->assertSession()->elementExists('css', '#bartik_branding');
}

}

Change issue status to "needs review" for the testbot to test the patch.

Rerolled and added tests-only patch.

Didn't realize the tests were incomplete.

Patch #24 tested and Applied successfully on Drupal 9.4.x.

Patch no. #24 tested and applied successfully on Drupal 9.4.x version.

#24 patch also working in D9.5

1. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
   @@ -244,6 +244,11 @@ public function testBlock() {
   +    $this->submitForm(['visibility[request_path][pages]' => 'user/login'], 'Save block');
   

   I think the test would be better if we entered two paths with only the second path missing a leading slash.

2. +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -112,6 +112,22 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    $paths = explode("\r\n", $form_state->getValue('pages'));
   

   I think this should use the same as the logic as the summary method.

   $paths = array_map('trim', explode("\n", $this->configuration['pages']));
   

   The explode "\r\n" looks like it might break on some systems.

3. +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -112,6 +112,22 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +      if (empty($path) || $path === '<front>' || strpos($path, '/') === 0) {
   

   strpos($path, '/') === 0 should be str_starts_with($path, '/') - this will work on both D9 and D10 as we have a PHP 8 polyfil.

4. +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -112,6 +112,22 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +        $form_state->setErrorByName('pages', $this->t('Paths require a leading forward slash when used with the Pages setting.'));
   

   I think the error should contain which path has caused the problem.

Addressed the points in #41

41.2 had to use form_state or couldn't get past the error once it triggered.

Fixed test case

1. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
   @@ -244,6 +244,15 @@ public function testBlock() {
   +    $pages = [
   +      '/valid-page',
   +      'user/login',
   +    ];
   

   Let's make this array

       $pages = [
         '',
         '<front>',
         '/valid-page',
         'user/login',
       ];
   

2. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
   @@ -244,6 +244,15 @@ public function testBlock() {
   +    $this->assertSession()->pageTextContains('user/login require a leading forward slash when used with the Pages setting.');
   
   +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -112,6 +112,22 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +        $form_state->setErrorByName('pages', $path . ' ' . $this->t('require a leading forward slash when used with the Pages setting.'));
   

   $path should be a placeholder in the string. Something like
   '%path requires a leading forward slash when used with the Pages setting.'

Addressed points in #45

+++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
@@ -112,6 +112,22 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+        $form_state->setErrorByName('pages', $this->t("The path '%path' requires a leading forward slash when used with the Pages setting.", ['%path' => $path]));

I don't think the quotes are necessary. The % wraps the text in em tags already. I think this is inline with other validation messages.

Updating patch as suggested in comment #47

@narendra.rajwar27 thank you for the patch but if you are going to contribute please upload an interdiff even for small changes.

Going back to #46

Queuing for 10.1.x tests.

Verified and tested patch #50 on 10.1.x-dev. Patch applied successfully.

Test Steps:
1. Goto Structure > Block Layout
2. Configure any block and select "Pages" tab under Visibility
3. Enter following URL paths - , valid page and "user/login"
4. Click "Save block"

Test Result: Proper validation error message is shown for missing leading forward slash specifying the URL path giving error.

Question: This works only for first path having missing leading forward slash. Should it display all the paths having missing forward slash ?

+++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
@@ -112,6 +112,22 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      else {

The else does nothing here, so let's drop it and the return for simplicity sake, less code++.

There's a new string here, so I think that makes this 10.1.x only, tagging as such

Removed else

This looks RTBC to me now, assuming the bot agrees

Random fail

Another random failure

Committed e47f5e7 and pushed to 10.1.x. Thanks!

Was looking at this issue and not sure why larowlan and sonam and maybe one or two others didn't get issue credit... but it's closed now so :shrug: :)

I believe I didn't credit @sonam.chaturvedi because at the time of #55 there was already automated tests that verified that the message was triggered. I missed that there was a question in the comment which looking at it now probably should have been addressed. I opened a follow-up for that since I think it's a good suggestion for usability #3356462: Validate all invalid paths in block visibility.

We usually give credit for non-trivial code reviews. It's hard to evaluate how substantial a review is but this time it looks like I decided that the feedback from @larowlan wouldn't qualify for the credit.

Thank you for the explanation. I’ll reassess how we decide to do manual testing in the future.

A contrib module called Block Exclude Pages was relying on the bug behavior.

#3375092: Drupal 10.1 + Rejecting paths not conforming to standard URL's set by Drupal