Fixes MarketingCampaignsReporting counting visit as new visit due to campaign_name keys mismatch

[AltamashShaikh]

Description:

Fixes MarketingCampaignsReporting counting visit as new visit due to campaign_name keys mismatch
Fixes: #PG-3672,matomo-org/plugin-MarketingCampaignsReporting#157

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[michalkleiner]

Would this also work? Just removed the extra variable and used a shorthand array directly in the loop.

Do I understand it correctly that we are replacing the the values in the array defined at the top of this method and we need to maintain an exact order there otherwise it wouldn't work?

Suggested change

	// If MarketingCampaignsReporting is active, it can override this params and if we don't account the changes here, the referer_name and referer_keyword keys would differ and lead to count as a new visit
	$marketingCampaignReportingKeys = array('campaign_name', 'campaign_keyword');
	foreach ($marketingCampaignReportingKeys as $index => $marketingCampaignReportingKey) {
	$campaignFields = StaticContainer::get('advanced_campaign_reporting.uri_parameters.' . $marketingCampaignReportingKey);
	if (!empty($campaignFields[$marketingCampaignReportingKey])) {
	$return[$index] = implode(',', $campaignFields[$marketingCampaignReportingKey]);
	}
	}
	// If MarketingCampaignsReporting is active, it can override these params and if we don't account for the changes here, the referer_name and referer_keyword keys would differ and lead to being counted as a new visit
	foreach (['campaign_name', 'campaign_keyword'] as $index => $marketingCampaignReportingKey) {
	$campaignFields = StaticContainer::get('advanced_campaign_reporting.uri_parameters.' . $marketingCampaignReportingKey);
	if (!empty($campaignFields[$marketingCampaignReportingKey])) {
	$return[$index] = implode(',', $campaignFields[$marketingCampaignReportingKey]);
	}
	}

[AltamashShaikh]

@michalkleiner Yes you are correct or else we can change the above array as below and at last do `array_values($return);, in this way we don't need to rely on index as key.

$return = array(
            'campaign_var_name' => Config::getInstance()->Tracker['campaign_var_name'],
            'campaign_keyword_var_name' => Config::getInstance()->Tracker['campaign_keyword_var_name'],
        );

[michalkleiner]

Yeah, I think that going with a mechanism with explicit string array keys is easier to understand, explicit, and less prone to errors that could come out from e.g. code refactoring.

On the other hand I'm not too sure if core should have a dependency/know about marketing campaigns reporting plugin, so maybe a hook here would be better and then the plugin can adjust the values as it needs?

What do you think about the approach here @matomo-org/core-reviewers ?

[sgiehl]

I'll repeat what I had messaged internally on slack already:

We actually shouldn't include any plugin specific handling to core for plugins that aren't included in core.
Also that fix would only be a partial fix. The plugin detects a campaign also with other parameters, like campaign source or medium, which core doesn't. That could also lead to incorrect starting of new visits.
A proper solution would be to finally work on #20067

[sgiehl]

I'll close this one for now. We should aim to work on #20067 instead.