Wrong URL for script in MatomoTagManager

Prerequisites

I checked for duplicate issues (open and closed)
I am using the latest stable version/release of the Contao Cookiebar
This issue is not a question, not a feature request and I have read the documentation and pinned issues

Please select the topic(s) that most closely match your concern

Backend (PHP)

Description

When using MatomoTagManager as cookie type, the url to load the javascript from the Matomo instance is not correctly set, as an additional "https://" is added.

In https://github.com/oveleon/contao-cookiebar/contao/dca/tl_cookie.php#L154 the requirement for the field is a full url (with "http://" or "https://").
In https://github.com/oveleon/contao-cookiebar/src/Cookie.php#L316 an additional "https://" is added in front of the already valid/full address.

I think this is far to small for a PR, but if you want one, I'll be happy to provide one. 🤣 But might not be as fast as you've been this afternoon. 😉

Thanks! 😄

Relevant log output

No response

I did add d81a275 to prevent #208 but that also makes people use a protocol...

Since this one is broken already and http/https is also valid within 'rgxp' => 'url', I think the correct approach would be changing

contao-cookiebar/contao/dca/tl_cookie.php

Line 154 in f279575

    
           'eval'                    => ['rgxp'=>'httpurl', 'mandatory'=>true, 'maxlength'=>255, 'tl_class'=>'w50'],

from httpurl to url.
It was a bug beforehand so unsure if a Migration is needed right now...

Feel free to open up a PR against the 1.x branch as I can't really test Matomo :)

The fix in #208 is perfectly fine and necessary, imho. People should put in an absolute url in that field, even if the url could be relative. In other words, #208 solved a bug, it just didn't went far enough. 🤣

I'd really not change the httpurl in the dca, as it is working for the "normal" Matomo code, just not the Matomo Tag Manager. And the protocoll shouldn't be hardcoded anyway. It should not affect running installations either, so I'd go with deleting the protocol in Cookie.php. 👍

I'll setup a PR later today. 😄

I'd remove https:// from the string in line 316 and then add:

if (!str_starts_with($url, 'http')) {
    $url = 'https://'.$url;
}

I'm obviously not getting it, but imho the protocol is not needed anymore. #208 did change the possibility to even set an url without a protocol in the dca. So in theory, there shouldn't be any url without a protocol. What, in theory, would make the protocol in #L316 unnecessary.

The only caveat I can see, if there already is an url saved without a protocol (from before #208), that would need to be taken care of aka the user would need to change that. With every try to save an edited dca, the regxp (=httpurl) filter should kick in and wouldn't let the user save the dca without a protocol.

Imho, the deletion of the protocol in #L316 would be the correct way. No additional check needed.

And not to forget, the "normal" Matomo cookie link is correct with httpurl, it is only the Matomo Tag Manager cookie, that isn't correct because of the additional "https://".

Or am I totally wrong here? Sorry if so! 😄

PS: I already did setup the PR and for the last few hours, it's running fine with just the change to delete #L316. 👍 If it stays that way, I'll send in the PR later this evening.

So in theory, there shouldn't be any url without a protocol.

No, that's not true. If you had the cookiebar installed before this change, your database will contain values without https://. Hence you need to check for https:// in the code.

Thanks for your input, @fritzmg, much appreciated! I'm unfortunately not overly familiar how these things are handled in Contao, sorry! I personally would change the entries in the db during the update to this version, so every entry in the db would be with the protocol in front. That would align the entries in the db with the filter that is set in the dca (httpurl). But every project handles such things differently. 😄

So, I'll push the PR with the deletion of #L316 and the additional check for a protocol later.

Thanks guys! 😄

I personally would change the entries in the db during the update to this version

Writing a migration
vs.
making sure it just works

Pick one because some people don't care for migrations if their front-end doesn't throw a 5xx error ;), their current Matomo Tag Manager would simply not work.

I agree with the simple solution that fritz proposed.

str_starts_with

I'd also propose to use something like strpos when targeting this against 1.x since it's still for PHP 7.4.
There may or may not be a symfony php 8 polyfill installed but let's keep it at the language level it's supposed to have.

I don't want to argue with people that are still on PHP 7.4 (and even on Contao 4.9)

I'm unfortunately not overly familiar how these things are handled in Contao

This has nothing to do with Contao itself.

I personally would change the entries in the db during the update to this version, so every entry in the db would be with the protocol in front. That would align the entries in the db with the filter that is set in the dca (httpurl). But every project handles such things differently. 😄

Sure - but implementing a Migration is a little more involved than adding the aforementioned 3 lines of code. But that's up to you ;)

I'd also propose to use something like strpos when targeting this against 1.x since it's still for PHP 7.4.

No need, you can just require symfony/polyfill-php80.

Sure - but implementing a Migration is a little more involved than adding the aforementioned 3 lines of code. But that's up to you ;)

😄 yup

No need, you can just require symfony/polyfill-php80.

but only in 1.x

Let's make it a 4 line PR then

but only in 1.x

Yes, when merging upstream you can simply remove that dependency - and the code can be kept the same.

Thanks a lot! You're practically serving me this on a silver tablet! 👍 🤣

See #227