In the Pathauto options, you are given three choices with punctuation marks: 1) remove 2) replace with separator, 3) no action.

However, if you choose the "remove" option, the punctuation mark is instead always replaced with the separator.

In fact, the help text for the separator symbol says the following: "Character used to separate words in titles. This will replace any spaces and punctuation characters. Using a space or + character can cause unexpected results."

These two options seem to be in conflict. Why have the option to remove a punctuation character if it will always be replaced by the separator?

For instance, if I've set the single quote punctuation to be removed, then the title "drupal's name" SHOULD result in "drupals-name" in the alias. Instead, "drupal's name" always renders as "drupal-s-name"

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zoon_unit created an issue. See original summary.

kpoornima’s picture

Assigned: Unassigned » kpoornima

I am looking in to it.

basicmagic.net’s picture

i can confirm that I am also experiencing this same issue, using 8.x-1.0-alpha1+9-dev.

kpoornima’s picture

By applying this patch issue can be resolved.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/src/AliasCleaner.php
@@ -181,9 +181,9 @@ class AliasCleaner implements AliasCleanerInterface {
+            $this->cleanStringCache['punctuation'][$details['value']] = '';
+            break;
+			
           case PathautoGeneratorInterface::PUNCTUATION_REPLACE:

nice find. has trailing spaces on the empty line after.

Looks like we're missing test coverage here. Both for the form and where it's used.

kpoornima’s picture

dermario’s picture

The following patch contains the fix by @kpoornima in #6 plus a number of Simpletests, that check if the settings-form is working. Theses tests also cover the removal of punctuation.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  1. +++ b/src/Tests/PathautoSettingsFormWebTest.php
    @@ -0,0 +1,229 @@
    +
    +    // Allow other modules to add additional permissions for the admin user.
    +    $permissions = array(
    +      'administer pathauto',
    +      'notify of path changes',
    +      'administer url aliases',
    +      'create url aliases',
    +      'administer nodes',
    +      'bypass node access',
    +    );
    +    $this->adminUser = $this->drupalCreateUser($permissions);
    +    $this->drupalLogin($this->adminUser);
    

    Code is fine but the comment seems unnecessary, I don't see how it allows other modules to add permissions, it's also not necessary. I'd just leave out the comment.

  2. +++ b/src/Tests/PathautoSettingsFormWebTest.php
    @@ -0,0 +1,229 @@
    +    $edit = array_merge($this->defaultFormValues + $this->defaultPunctuations, array('verbose' => '1'));
    +    $this->drupalPostForm('/admin/config/search/path/settings', $edit, t('Save configuration'));
    

    You don't have to submit defaults, simpletest will use existing default values automatically for all fields that you don't submit explicitly.

  3. +++ b/src/Tests/PathautoSettingsFormWebTest.php
    @@ -0,0 +1,229 @@
    +    // Ensure the character case setting works correctly.
    +    // Leave case the same as source token values.
    +    $this->checkAlias('My awesome Content', '/content/My-awesome-Content', array('case' => '0'));
    +    $this->checkAlias('Change to Lower', '/content/change-to-lower', array('case' => '1'));
    

    Ah, now I see, you use the defaults to restore them after changes. Fair enough, that makes sense here. Not realy needed above where you only have a single submission but I guess it also doesn't hurt.

  4. +++ b/src/Tests/PathautoSettingsFormWebTest.php
    @@ -0,0 +1,229 @@
    +
    +    $ignore_words = 'a, new, very, should';
    +    $this->checkAlias('a very new alias to test', '/content/alias-to-test', array('ignore_words' => $ignore_words));
    

    Once this is in, would be awesome if you could extend this test to also provide a test for #2656958: Strings to remove default setting is incorrect, meaning, just making sure that the values shown in the form are the expected defaults. You will have to do that first in a test method or a separate one, so that you don't already overwrite them.

Overall, nice work! So the only thing to fix is the comment. If you do, please also provide a -test-only.patch (that you upload first, then the full patch). That way, we can easily prove that the test is covering the bug and fails without the fix.

dermario’s picture

Thank you for your quick and detailed feedback. I made the following adjustments to the tests:

  1. Removed the comment in setup()
  2. Introduced an new method testDefaultFormValues() to verify to form values are correct on initial load.
  3. Removed the array merge construction in testVerboseOption() to use the default form values there.

Attached is the test-only-patch. This patch is supposed to fail. My next comment will contain the complete (hopefully) succeeding patch.

Im am not sure how to deal with #2656958. My patch can cover that (with a small a adjustment). Do we need to fix this punctuation issue first and adjust the test for #2656958 afterwards to avoid conflicts?

Status: Needs review » Needs work

The last submitted patch, 9: punctuation_not_removed-test-only-2674494-9.patch, failed testing.

The last submitted patch, 9: punctuation_not_removed-test-only-2674494-9.patch, failed testing.

dermario’s picture

Here is the complete patch.

Berdir’s picture

Status: Needs review » Fixed

Yes exactly. We commit this and then you adjust the test over there so that we have that bug covered too.

One more thing, when making changes to a patch, you should always provide an interdiff file that is basically a diff against the previous version of the patch. Then I see exactly what changed, otherwise I have to look through a possibly long patch to figure out what exactly changed.

The easiest way to create those (IMHO) is to have a branch per issue and then a commit for each version of a patch. Then diff against 8.x-1.x for the full patch, against HEAD~1 for the last commit that is then the interdiff. And upload both in the same comment.

Just information for the next time, you no longer have to do it here. Committed, thanks!

  • Berdir committed bf27fb6 on 8.x-1.x authored by dermario
    Issue #2674494 by dermario, kpoornima: Punctuation not removed correctly
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.