Allow to trim special characters from the end

I fixed the update script as it had been hidden inside a different update script; also, it should have been named metatag_update_8110().

The last submitted patch, 15: metatag-n3311415-15.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

@DamienMcKenna thanks! Any comments on #13?

Perhaps @Grevil can take a final look here in the next days then.

Yikes, found a pretty embarrassing bug, leading to the trimByMethod() not working properly, all fixed now, tests are green. Please review once again!

LGTM!

Just two things:
1. I'm just not totally sure if this part

str_replace("\\", "\\\\", $trimEndChars)

is the best solution, but also have no better idea yet.

2. Did you test (manually), that the update hook with that number runs? I agree with your documentation (while @DamienMcKenna may of course decide to keep the 8 in front). But I also remember there were some flaws with these update hook numbers, so we should be 100% sure this works at least :)

Settings this RTBC for now, but would be nice if @DamienMcKenna could have a look and @Grevil could give some final feedback on the points.

I'm sticking with the 8xxx series of the update scripts following the branch number convention, not the core number convention, as it keeps them more consistent. I also spotted that the update script wasn't running $config->save() after setting the value, and improved the settings form's wording.

I wonder if we should expand the default list of characters to remove, #13 provided a reasonable list of additional ones that would be worthwhile to add as a default.

I'm sticking with the 8xxx series of the update scripts following the branch number convention, not the core number convention, as it keeps them more consistent.

Fine! :)

I also spotted that the update script wasn't running $config->save() after setting the value,

Thank you for spotting this. ouch!! Painful mistake!

I wonder if we should expand the default list of characters to remove, #13 provided a reasonable list of additional ones that would be worthwhile to add as a default.

As I suggested these, someone else should please decide. I personally like the idea.

FYI: "Path" module solves this differently, using selects. And it has an evolved list:
/admin/config/search/path/settings

Should we decide based on this perhaps? What about the UI/UX? I don't think we need selects or checkboxes in our case and the input is more flexible?

Any other defaults to add?

I guess it's fine if you decide @Damien!

Grevil → changed the visibility of the branch 2.0.x to hidden.

Grevil → changed the visibility of the branch 3311415-allow-to-trim to hidden.

I manually added 2.0.x to this issue fork, added an appropriate 3311415-2.0.x issue branch, created a new MR targeting 2.0.x, applied all 8.x targeted changes to the new issue branch and additionally applied the changes from Damiens interdiff in #21 ✨ Allow to trim special characters from the end Needs work .

I'll manually test the functionality in a couple of days and provide code additions concerning #22. Until then, I keep this on "needs Work".

All done and tested! :)

Please review!

Great work @Grevil once again! Code LGTM and all feedback seems to have been addressed.

Still, I have two things to discuss regardin the wording and UX:
1. We're no native speakers, so it would be great if @DamienMcKenna could review and improve all the texts.

2. I'm not totally happy with the UX of the added <select> value.
The select value "Only trim specified characters at the end of the Meta Tag" tells me something different then the description of the field:
"The trimming is applied after the tag is trimming for length, and after the trimming option was executed"

Furthermore I think you'd like to combine the existing options with the new optional character trimming, not have it as alternative? Currently it seems xor from the UI and that seems strange to me and misses the real world use case, where you'd always like to trim these characters from the end, if configured. Also if they appear at the end after length trimming, which is quite typical.

Example:
My quite long page title | My wonderful sitename
might be truncated to My quite long page title |
and in such cases you don't want the title to end with |! That's one of the most important use-cases.

So if I don't get things wrong, we either

• need an additional checkbox to enable the character trimming instead of the select value
• or need to leave the default empty and provide the suggested example values so it's used if filled.

Definitely agree, that the fourth option is confusing (and not needed)!

We can simply leave the maximum length option empty and achieve the same output! I wonder why, though this was a good idea in the original patch. :D

Adjusted accordingly. Note that the texts were already coming from Damien originally.

Great! RTBC from my side, let's hear what @DamienMcKenna says and finish this helpful feature :)

We'll add this to 2.1.0.

Sounds good to me!

Static patch attached, because I'm afraid 2.1.0 will need some more months. (Still hope it might land earlier :P ;))

@Grevil: Just ran into the following issue in production:

TypeError: Drupal\metatag\MetatagTrimmer::trimEndChars(): Argument #2 ($trimEndChars) must be of type string, null given, called in /web/modules/contrib/metatag/src/MetatagTrimmer.php on line 129 in Drupal\metatag\MetatagTrimmer->trimEndChars() (line 81 of modules/contrib/metatag/src/MetatagTrimmer.php).

which is called from

metatag/src/Plugin/metatag/Tag/MetaNameBase.php on line 659

After setting a value in the characters to trim field, it works fine!
So I guess we should add valuable defaults or have to change the string check!

Whoops, the new install hook was inside the brackets of the older install hook...

Fixed it again. Static patch provided.

Perfect, thank you @Grevil! Working as expected now. Back to RTBC and ready for the go :)

The MR needs to be updated to work off the 2.1.x branch.

Done.

Go! :)

@damienmckenna maybe this could be merged into 2.1.x-dev already? (I'm very much looking forward to the 2.1.0 release as this is a major SEO improvement and simplification for many of our projects :))

Committed.

Thank you soooo much @damienmckenna!! This is great news for us!

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