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"
Comment | File | Size | Author |
---|---|---|---|
#12 | punctuation_not_removed-2674494-10.patch | 9.97 KB | dermario |
#9 | punctuation_not_removed-test-only-2674494-9.patch | 8.62 KB | dermario |
#7 | punctuation_not_removed-2674494-7.patch | 9.28 KB | dermario |
#6 | punctuation-not-removed-correctly-2674494-6.patch | 1.35 KB | kpoornima |
Comments
Comment #2
kpoornima CreditAttribution: kpoornima as a volunteer and at Melity commentedI am looking in to it.
Comment #3
basicmagic.net CreditAttribution: basicmagic.net commentedi can confirm that I am also experiencing this same issue, using 8.x-1.0-alpha1+9-dev.
Comment #4
kpoornima CreditAttribution: kpoornima as a volunteer and at Melity commentedBy applying this patch issue can be resolved.
Comment #5
Berdirnice 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.
Comment #6
kpoornima CreditAttribution: kpoornima as a volunteer and at Melity commentedUpdated patch.
Comment #7
dermarioThe 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.
Comment #8
BerdirCode 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.
You don't have to submit defaults, simpletest will use existing default values automatically for all fields that you don't submit explicitly.
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.
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.
Comment #9
dermarioThank you for your quick and detailed feedback. I made the following adjustments to the tests:
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?
Comment #12
dermarioHere is the complete patch.
Comment #13
BerdirYes 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!