Make mtm_cid case sensitive

[chopsol]

I'd like to recommend changing the value for mtm_cid to respect case so it can be used for gclid tracking for Google Ads.
See also #27

[diosmosis]

Thanks for the fix @chopsol! I requested a change, but for the most part it looks good to me. I'm not sure if we can merge it just yet, though.

@sgiehl @tsteur do you know if we need to remove casing when aggregating? I assume having IDs that vary by case isn't something to expect in the wild (typos happen, surely, but maybe not often?). Can we merge this (after adding some test data)?

[tsteur]

@diosmosis I don't really know but maybe @sgiehl knows more? Generally I would have expected every parameter to be case sensitive. Not sure regarding casing when aggregating.

[chopsol]

I changed my request by creating a new parameter mtm_gclid to make sure that old behaviour don't break.

[diosmosis]

@chopsol thanks for updating the pr!

@tsteur do you see a problem with supporting a new parameter for this use case?

[tsteur]

@diosmosis I don't really know much about it unfortunately. @sgiehl can you have a look at this too?

[sgiehl]

@tsteur I guess I would actually also have expected the parameters to be case sensitive. But seems core is handling it the same way for campaign names, so not sure if we should change that in general. Maybe we could introduce a new config setting to allow changing that behavior?
Personally I wouldn't add a specific parameter to handle such special clickids case sensitive. (But if, we shouldn't name it mtm_gclid. gclid is only the parameter of Google, but that would also be fbclid, mscklid or yclid for other providers. So a better name might be mtm_clid or mtm_clickid.)

For tracking click ids it actually would also be possible to simply use a custom dimension. that should already be case sensitive.

[chopsol]

@sgiehl Good point. I changed the parameter to mtm_clid

[chopsol]

Are there any reasons not to merge this pull request?

[sgiehl]

Personally I'm still unsure if we should make a strtolower on the campaign parameters at all. I can't see any benefit in it. If we want to create the reports case insensitive, we could also do that when aggregating the data. As mentioned before, that might also be the case in core... @mattab @tsteur any opinion here?

Btw. we have released a new premium plugin, which automatically tracks click id and allow segmenting for them. See https://plugins.matomo.org/AdvertisingConversionExport

[chopsol]

Changing this main behavior within minor update may break reportings.
If we change this, it should be configurable.
If this is a desired changed, it can be developed by me.

[mattab]

Here are my thoughts:

• Ideally we would store in the DB, the value as-is (no change except a trim or so)
• Ideally during aggregating, we would aggregate values to the "canonical" value. For example maybe the campaign name is mostly written as "My_Campaign" so ideally we would report it as "My_Campaign" (so for example "my_campaign" would also be grouped under the main "My_Campaign").
• Changing it by default for everyone would be a BC break, we couldn't really do it until Matomo 5. But also it wouldn't be really useful, because it does happen that the case is lost in these parameters, so there is no avoiding the issue (and having duplicates with different cases would be a bad reporting experience...)

[sgiehl]

@mattab couldn't we remove the strtolower from those parameters in this plugin and in core, but change the archiving so it would do a strtolower on the label there. That way the aggregated reports shouldn't change. In theory we could even make it configurable if a strtolower should be done in archiving, so it's possible to change that behaviour

[mattab]

Sounds good @sgiehl as a simpler solution, that wouldnt break BC.

[chopsol]

I'd like to push this request once more. Are there any reasons not to merge? I understand that a greater change will be nice, but until then, this change doen't break BC either.

[diosmosis]

ping @sgiehl

@chopsol Apologies, I won't be able to make a final decision on this.

[sgiehl]

@justinvelluppillai It should be fine to merge this PR. Nevertheless we should consider building a proper solution as I mentioned some comments above. Even though it won't be a BC break, we could also plan this for Matomo 5.