Adding support for URL shortener in email 5.x

[christian-krieg]

This PR migrates PR #12111 from 4.x to 5.x. Compared to #12111, this PR adds:

• Adding assertions to unit test
• Twig template instead of PHP template

Please note that CS Fixer introduced whitespace in some files in order to align assignment characters ('=', '=>'), so the committed changes are distorted (compared to the functional changes).

Q	A
Bug fix? (use the a.b branch)	No
New feature/enhancement? (use the a.x branch)	✅
Deprecations?	No
BC breaks? (use the c.x branch)	No
Automated tests included?	✅
Related user documentation PR URL	mautic/mautic-documentation#...
Related developer documentation PR URL
Issue(s) addressed

Description:

Steps to test this PR:

1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

[codecov]

Codecov Report

Merging #12128 (0193d4a) into 5.x (1635d74) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##                5.x   #12128   +/-   ##
=========================================
  Coverage     58.00%   58.00%           
- Complexity    33536    33539    +3     
=========================================
  Files          2163     2163           
  Lines        101335   101341    +6     
=========================================
+ Hits          58777    58781    +4     
- Misses        42558    42560    +2     

Impacted Files	Coverage Δ
app/bundles/CoreBundle/Form/Type/ConfigType.php	100.00% <100.00%> (ø)
...es/EmailBundle/EventListener/BuilderSubscriber.php	97.14% <100.00%> (+0.06%)	⬆️

... and 2 files with indirect coverage changes

[npracht]

🚀 thanks a lot !

[escopecz]

I see no issue with these changes. Quite elegant solution 👍

[escopecz]

But please fix the CI issues.

[christian-krieg]

But please fix the CI issues.

@escopecz I cannot see where the 'CS Fixer' check fails, could you please provide assistance?

[escopecz]

Sure. It's visible when you scroll down that "Some checks were not successful" and you can click on details to see what's failed. Here is the direct link: https://github.com/mautic/mautic/actions/runs/4610697990/jobs/8305732563?pr=12128

[christian-krieg]

Thanks @escopecz , I already managed to go that far :) I just can't see what causes the issue, or, more generally, what is the issue, or where to start bug hunting.

[escopecz]

I can help with that. The CS fixer makes sure that the code style is the same across the code base. We configured it to be very similar to what Symfony code style is plus some additional rules.

The output shows you lines of code. If the line starts with - then it wants to remove it and when it starts with + then it wants to add it. So you can manually fix your code and push it.

But the CI job also shows what command it runs on the first line. You can run it on your local like this:

bin/php-cs-fixer fix --config=.php-cs-fixer.php

and it will fix the code style issues for you. Then you just review it and push it.

[christian-krieg]

Hmmm it seems CS Fixer is complaining about code I never edited...

EDIT: And now, all checks pass 😅

[christian-krieg]

Hey there, is there any chance to get this PR merged soon? Is there anything I can help with?

[escopecz]

@christian-krieg it needs to be tested and approved by a second person. Do you think you can do that?

[christian-krieg]

Of course I tested before submitting, and I successfully have been using this feature for months now. However, I am not sure if it complies with the intended four-eyes principle if I am the one who tests and approves. But of course, I am willing to do so.

[escopecz]

Oh, you are the author. I thought this is from @kuzmany but I can see it's different PR:

#12299

@kuzmany can you please look into this PR and approve if it's OK with you? I don't want these 2 PRs to conflict not only in code but in UX.

[kuzmany]

@escopecz once If ready you can merge it and I will update my PR

But I see one issue. After this PR we will have a switch button for shortener URLs for emails, but on SMS it's automatically enabled. Probably we should have a switch button also for SMS. But in the end I can add it in my PR and cleanup

[christian-krieg]

Probably we should add it for all available channels then? By the way, in my opinion it makes sense to auto-enable it for SMS, since the tracking links are ultra-long, likely to span multiple messages.

[escopecz]

Ok. I'll wait for the second approval

[volha-pivavarchyk]

Works well as discribed in PR #12111.

[escopecz]

@christian-krieg please resolve the conflict

[christian-krieg]

@escopecz The "Mark as resolved" button is grayed out; It seems that I am not able to do this.

[escopecz]

It's better to do this on your local by either rebasing to the fresh 5.x branch or merging it. Rebase is a cleaner way if there are not many conflicts. Let me know if you need more details on how to do this.

[escopecz]

@christian-krieg could you please resolve the conflict so this PR could be merged? It's also blocking #12299 which needs to go into 5.0.0-beta so it must get in within a week or so.

[mautibot]

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/mautic-5-beyond-expectations-beyond-limits/30459/1