Path comparison (e.g. for block visiblity) doesn't work for aliased internal paths with capital letters (block_block_list_alter() in D7)

This patch makes the behaviour of the function match the documented behaviour. RTBC.

Nice patch, but I think this is really worth writing a test for. Here's a start.

Also, has anyone checked whether this bug affects Drupal 8?

Hello @M_Z,
I have verified that issue is still there in D8, so kindly review a new patch for D8

I am working on this issue

As @M_Z points out we need an automated test.

I also wonder if we should make \Drupal\Core\Path\PathMatcher::matchPath() do a case insensitive match ie change
$this->regexes[$patterns] = '/^(' . preg_replace($to_replace, $replacements, $patterns_quoted) . ')$/';
to
$this->regexes[$patterns] = '/^(' . preg_replace($to_replace, $replacements, $patterns_quoted) . ')$/i';

and thereby

+++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
@@ -157,7 +157,7 @@ public function evaluate() {
     $path_alias = mb_strtolower($this->aliasManager->getAliasByPath($path));

Remove the mb_strtolower() when you create the $path_alias

I think this is very much inline with #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing

Fixing it in \Drupal\Core\Path\PathMatcher::matchPath() would help contrib modules (and custom code) - for example see \Drupal\redirect_404\EventSubscriber\Redirect404Subscriber::onKernelException...

        if ($this->pathMatcher->matchPath(mb_strtolower($path_to_match), $pages)) {
          return;
        }

working on it.

Adding test case. Kindly review a new patch.

@Hardik_Patel_12 can you please add a test only patch (one with just your test in it and not the fix) so we can see that this is accurately reproducing the bug

1. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -6,10 +6,13 @@
   +  /**
   +   *
   +   */
      public function testMultipleForms() {
   

   this is an unrelated change, no need to do that in this patch

2. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -37,4 +40,14 @@ public function testMultipleForms() {
   +  /**
   +   *
   +   */
   +  public function testblock() {
   

   this seems to be missing some text and maybe a more descriptive name?

3. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   +    $this->drupalGet('admin/structure/block/add/' . $block_name . '/' . $default_theme);
   +    $this->drupalPostForm(NULL, $edit, t('Save block'));
   

   no need to Get first, you can just use the path as the first argument in drupalPostForm instead of NULL

@Lendude Addressed suggested changes,
Please review

Some more feedback.

1. Can we get a test-only patch please (see #31)

2. +++ b/core/modules/block/tests/modules/block_test/block_test.routing.yml
   @@ -5,3 +5,10 @@ block_test.test_multipleforms:
   +    _controller: '\Drupal\block_test\Controller\TestMultipleFormController::testblock'
   
   +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -37,4 +37,14 @@ public function testMultipleForms() {
   +   * Testing page block.
   ...
   +  public function testblock() {
   

   Can we come up with something a little bit more descriptive then 'testblock'

3. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -6,7 +6,7 @@
   - * Controller for block_test module
   + * Controller for block_test module.
   

   unrelated change

4. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   +    // Check on front page block is not placed.
   ...
   +    // Check on 'BlockTestPageWithCapitalLetters' page block is placed.
   ...
   +    // CHeck on alias page block is placed.
   

   These comments need to be full English sentences, these are a bit off.

added the test only patch @lendude, will work the wordings. So #1 is fixed, working on others.

updated the wordings as mentioned in the #2 , #3 and #4 @Lendude

Patch at 39 looks good to me and it has covered all points suggested at #35.

Thanks for working on this 💪

1. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -6,7 +6,7 @@
   - * Controller for block_test module
   + * Page with Capital Lettered Url to test block's page visibility.
   

   This change is out of scope here

2. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -37,4 +37,14 @@ public function testMultipleForms() {
   +      '#markup' => $this->t('Test Block for Page with Capital Letters .'),
   

   this isn't a block, its a controller, not sure about the use of 'Block' in the markup, but its minor

3. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   +
   

   nit: this extra line isn't needed

Here I have addressed #46.1 and #46.3 points of comment #46 and line exceeding more than 80 characters issue.

Tested #47 on 9.1 -
1. #46.1 and #46.3 points of comment #46 are resolved
2. Line exceeding more than 80 characters issue is also resolved

Applied patch #47 on 9.2.x and it works fine. Including screenshots below.

Before (Block not displaying with given path containing capital letter) :

After( Block is showing now):

Came to do a review but the issue summary does not state the proposed resolution, tagging for an IS update. I think this could use a better title too.

@Abhijith S, it helps the reviewer to have the screenshots in the Issue Summary.

Then I looked at the code.

1. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -37,4 +37,14 @@ public function testMultipleForms() {
   +   * Test block visibility on a page with Capital Letters in the URL.
   

   s/Capital Letters/capital letters/

2. +++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
   @@ -37,4 +37,14 @@ public function testMultipleForms() {
   +      '#markup' => $this->t('Test Block for Page with Capital Letters .'),
   

   s/Letters ./Letters./

3. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   + * Tests block with capital letter path functionality.
   

   This could be clearer. Can't think of alternatives right now.

4. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   +    $this->assertNoText($title, 'Block was not displayed on the front page.');
   

   Remove the message in this assertion and the other assertions.

5. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   +    // Test the page with url 'BlockTestPageWithCapitalLetters' where block
   +    // should be visible.
   

   I think 'Test that the block is visible on /BlockTestPageWithCapitalLetters' is much easier to understand.

6. +++ b/core/modules/block/tests/src/Functional/BlockTestWithCapitalLetters.php
   @@ -0,0 +1,59 @@
   +    // Test the page with it's alias where block should be visible.
   

   I think 'Use the alias and test that the block is visible on /BlockTestPageWithCapitalLetters' is much easier to understand.

Here uploading my patch.

pls review it

Fixed test failures in #53, thanks!

This still needs an issue summary update, see Write an issue summary for an existing issue for guidance. Particularly, the proposed resolution - that allows the reviewer to confirm that the patch is doing what it is supposed to be doing.

Updating the IS is a suitable novice issue, adding tag.

Attached patch against Drupal 10.1.x

Fixed CCF on #63 and rerolled for Drupal 10.1.x

Removing credit from both #63 and #64 as #63 didn't check the patch before uploading and removed the fix and some of the tests. #64 carried it forward.

Also patch #55 still applies to D10.1 so a reroll was not needed.

#55 tests will have to be updated for D10.1

Test only patch

Test only patch

Updated the test as mentioned in #66

We shouldn’t use olivero in a test unless we have to.

An issue summary update is still needed.

FYI this issue was tagged for novice for new users. You have 26 pages of posts so definitely not novice. Please try to avoid novice issues in the future

Thanks!