PathAlias::preSave() should also trim the path, not just the alias

Hi @cm0dit,

Please make sure that the issue is assigned to someone else or not before adding a patch or comment. As I am also working on test case failures, but I just saw your comments.

Anyways, please run the test cases for the #5 patch.

working on it.

Hi @Hardik_Patel_12,

I have assigned this ticket to myself and you have changed the assignee and assigned to yourself. I think this is not the right approach. Please make sure that the ticket is not assigned to someone before working on that.

Thanks!

Kindly review a new patch.

Hi @ankit.singh , ticket was assign to you 4 days ago so that's why i thought you are not working on this. At least you should write that i am working on this issue so then only people will understand that not to pick this issue. So its a kindly request that if we are facing some trouble in the issues and spending couple of days then simply unassign the issue so other people can work on it.

Thanks!

1. Visit admin/config/search/path
2. Append the System path field value '/'

e.g. /node/1 will become /node/1/

Before Patch: It shows a 404 page not found an error.

After Patch: Patch applied successfully and the respective entity page opens up.

Good to move it to RTBC +1

I have re-run the #9 patch for Drupal version 8.9 and the patch applied successfully.

Thanks @ankit.singh , for verifying now its looks good to go. Moving to RTBC.

Thanks everyone for filing this bug report and for fixing the issue. Bug fixing is very valuable.

@ankit.singh, thank you for looking into to this issue. Posting screenshots of your codebase or CLI does not advance the issue, since the automated testing infrastructure tells us whether the patch applies correctly. However, the before-and-after screenshots of the UI are potentially helpful, so leaving credit for that. Next time, could you crop them only to the relevant part of the screen, and embed them in the issue?

So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose, including whether it completely fixes the issue, whether it is the best fix, and so on. Thank you!

1. +++ b/core/modules/path_alias/src/Entity/PathAlias.php
   @@ -94,10 +94,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -    // Trim the alias value of whitespace and slashes. Ensure to not trim the
   -    // slash on the left side.
   +    // Trim the alias and path values of whitespace and slashes. Ensure to not trim the
   +    // slash on the left side, but only the trailing slash.
   

   The comments here are not wrapping properly. We need to make sure the new comment is wrapped at 80 characters.

2. In order to commit a bug fix, we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

   1. https://www.drupal.org/docs/8/testing
   2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.9.x

3. +++ b/core/modules/path_alias/src/Entity/PathAlias.php
   @@ -94,10 +94,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        $this->setAlias($alias);
   +    $this->setPath($path);
   

   Looks like this is a difficulty between the setAlias() and setPath() methods on PathAlias.

   I checked their relative docs on the interface.

   setAlias() has:

     /**                                                                           
      * Sets the alias for this path.                                              
      *                                                                            
      * @param string $alias                                                       
      *   The path alias.                                                          
      *                                                                            
      * @return $this                                                              
      */
     public function setAlias($alias);
   

     /**                                                                           
      * Sets the source path of the alias.                                         
      *                                                                            
      * @param string $path                                                        
      *   The source path.                                                         
      *                                                                            
      * @return $this                                                              
      */
     public function setPath($path);
   

   ...That's not helpful to our situation, where we are trying to decide if this is an appropriate way to mess around with PathAlias::preSave(). However, it does sound like the trimming is supposed to be done on output, not input. So I'm not totally sure of changing the user's input here.

covering points suggested by @xjm .

#15.1 done
#15.2 adding test case for this bug
#15.3 i think changing the user's input here its ok , but yes i am also still not sure about this.

1. +++ b/core/modules/path_alias/src/Entity/PathAlias.php
   @@ -94,10 +94,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    // Trim the alias and path values of whitespace and slashes.
   +    //  Ensure to not trim the slash on the left side,
   +    //   but only the trailing slash.
   

   Wrapped too early.

2. +++ b/core/modules/path_alias/tests/src/Functional/PathWithSlashFunctionalTest.php
   @@ -0,0 +1,84 @@
   +   * Modules to enable.
   +   *
   ...
   +  protected static $modules = ['path'];
   

   Should be {@inheritdoc}

3. +++ b/core/modules/path_alias/src/Entity/PathAlias.php
   @@ -94,8 +94,9 @@
   +    // Trim the alias and path values of whitespace and slashes.
   +    //  Ensure to not trim the slash on the left side,
   +    //   but only the trailing slash.
   

   Wrapped too early

4. +++ b/core/modules/path_alias/tests/src/Functional/PathWithSlashFunctionalTest.php
   @@ -0,0 +1,84 @@
   +    $this->drupalPostForm('admin/config/search/path/add', $edit, t('Save'));
   ...
   +    $this->drupalPostForm('admin/config/search/path/edit/' . $pid, $edit, t('Save'));
   

   t() is unnecessary

5. +++ b/core/modules/path_alias/tests/src/Functional/PathWithSlashFunctionalTest.php
   @@ -0,0 +1,84 @@
   +    // Get last created path alias and update the path,
   +    // new path ends with slash(/).
   

   Wrapped too early.

6. +++ b/core/modules/path_alias/tests/src/Functional/PathWithSlashFunctionalTest.php
   @@ -0,0 +1,84 @@
   +  public function getPID($alias) {
   

   getPid()?

7. +++ b/core/modules/path_alias/tests/src/Functional/PathWithSlashFunctionalTest.php
   @@ -0,0 +1,84 @@
   +    return reset($result);
   

   What if it's []

Added test cases (Kernal).

@xjm Thanks for the suggestion. I think trimming supposed to do on input (not output) othewise every request It needs to trim (performance)

Please review

Well, #20 is a kernel test. not a funcational test as #16. So uploading a test only patch based on #20.

(Yeah, It's bad that the last patch is a test-only patch)

+++ b/core/modules/path_alias/src/Entity/PathAlias.php
@@ -94,10 +94,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
 
-    // Trim the alias value of whitespace and slashes. Ensure to not trim the
-    // slash on the left side.
+    // Trim the alias and path values of whitespace and slashes. Ensure do not
+    // trim the slash on the left side, but only the trailing slash.

this somehow changed from "to not" to "do not", that's not correct grammar, was right before.

@berdir, Thanks for focusing it out. I think it mistakenly added by me. will correct it

Fixed grammatical mistake in comment #20.

Now patch is looks good to me , grammar change is also covered as suggested by @Berdir , moving to RTBC.

Does this need an update to trim trailing forward slashes from existing data?

Yup, I think we need an update function.

OK let's add that here.

- Adding an update hook to re-save all the path aliases.

I am wondering if we need more validation around it to save only paths which ends with "/"
As, I'm just re-saving nodes, I haven't added tests for update hook.

@catch, please suggest if we want to refactor the logic to update existing path aliases.

Fixed Custom Commands Failed.

+++ b/core/modules/path_alias/path_alias.install
@@ -0,0 +1,17 @@
+
+/**
+ * Re-save all the path alias to removing trailing space and slash.
+ */
+function path_alias_update_9100() {
+  $path_aliases = \Drupal::entityTypeManager()->getStorage('path_alias')->loadMultiple();
+  /** @var \Drupal\path_alias\Entity\PathAlias $alias */
+  foreach ($path_aliases as $alias) {
+    $alias->save();
+  }
+}

This could be done in a hook_post_update_NAME(), since it's affecting content rather than schema. Also needs to use batch since there could be millions of aliases on a site.

Updated the patch in #33 addressing the changes suggested in #34, i.e., moved the update_9100() hook in path_alias.install to post_update_Name hook in path_alias.post_update.php.

Also tried to use the batch process within the post_update_Name hook, please review.

Thank you!

triggering for 10.1

Tested this on Drupal 10.1 with a standard install
Followed the steps in the issue summary and confirmed I was able to add a URL alias /node/1/
With node/1 having an alias of /hello-world.
I add a menu link to the main navigation for node/1
When I got to any page I see that the alias works fine.
Unless I missed a step

But agree it should cleanup the trialing /
Can the issue summary be updated with the proposed solution.

Thanks.

Added type hinting in the batch process and changed the limit from 25 to 50 in the post_update hook.

I've revised the issue description; please feel free to suggest any further changes if you think they are needed.