Route with space at the end does not work

Hi @smustgrave,

Can we now merge this patch into Drupal 10 branch if everything looks good to you.

Thanks & Regards,
Anup

I cannot review it as I helped work on it.

Verified the patch #28 and tested it on Drupal version 10.1.x. The patch works fine and I have added the before and after screenshots for reference.

Thanks, reviewed the patch at #28

For posterity \h in regex matches any horizontal space character

I wonder if this means something with a space in the middle of a path will no longer match?

Can we add an extra test case with /example/with%20a%20space (i.e. url encoded space) to ensure it still matches.

Added additional test and it didn't cause a failure.

+++ b/core/tests/Drupal/Tests/Core/Path/PathMatcherTest.php
@@ -150,6 +150,16 @@ public function getMatchPathData() {
+        '/example/1 ',
+        [
+          '/example/1' => TRUE,
+          '/example/2' => FALSE,
+          '/example/with%20a%20space' => FALSE,

Ah sorry, can we add a separate case where the URI matches the space

I.e we want it to be TRUE

      [
        // Test spaces.
        '/example/with a space',
        [
          '/example/with%20a%20space' => TRUE,
        ],
      ],

Seems this fails so maybe the solution needs work.

Kaushik1216 → made their first commit to this issue’s fork.

@Kaushik1216 can you explain how your MR is different to the existing patch?

Here is change and the test suggested on #36. I attached the interdiff as well.

CI failure

And the MR 3610 from what I can is the same fix but with the test file removed.

Sorry. New patch and interdiff.

@larowlan MR 3610 will remove space at start and end and previous patch remove all spaces in page url .

Reviewing patch #43 and seems the point brought up by @larowlan in 36 has been addressed.

The fail looks to be introduced here

There is confusion here about what is to be committed, the MR or the patch. Comment #45 refers to the patch but there is nothing in the issue summary to help the reviewer or committer to know what to look at. I have added the standard template to the Issue Summary and I am tagging this for an IS update.

@Kaushik1216, Welcome to Drupal! Thanks for the interest in this issue. if an issue is using a patch workflow then it is usually best to continue with that. Introducing an MR can cause confusion, as in this case. and sometimes duplication of work.

I disagree with the approach taken in the latest patches and would like us to still investigate what @catch wrote in #10.

There is a reason in #11 about why this is difficult, but I think that we should still go trough that direction. Anything that is changed in the path matcher has performance implications for every single request, and while regexes are very fast, if we can avoid changing them that would be better for performance I think.

Something like implode('\n', array_map(explode('\n', $input), function ($str) { return trim($str); })) (not actually tested, so not sure), should be enough in the form submit?
It would mean the tests added here should be removed as well and replace with a test that tries to submit the form and checks that the saved configuration is different (without the spaces).

That way, there is no code in the every single page request but only in the saving of the form.

Means we would have to tweak the test suggestion by larowlan in 33. As trim() doesn’t pass that