The fix actually looks fine to me. The test failures are related to that change, as some of the fixtures try to track visits providing the attribution data (_rcn) directly. Those visits are no longer attributed to those campaigns.

We nevertheless need to discuss this solution with @mattab and/or @tsteur in detail, as no longer using the attribution parameters to attribute visits is actually a breaking change. Even though it should also fix a couple of other issues we had around differing detection with core and MarketingCampaignsReporting plugin.

@mattab can you have a look at this one?

@bx80 are you aware of any specific reports that would no longer work or anything that will break or any behaviour that changes that people may have expected to work previously? Do I see it right that now only otherwise _rcn is used to get the value for a goal?

@tsteur Yes, _rcn should only be used for goals now. I don't know of any reports that would break because of this change. It looks like up until fairly recently the _rcn parameter wasn't being sent by the JavaScript tracker, so ignoring it for referrer campaign detection would restore the previous behavior in this case.

@bx80 That is not fully correct. The attribution parameters like _rcn have already been sent before #19200. That PR should have only improved the detection if no pageview is being tracked before the user leaves the page.
Nevertheless I actually thought before that those attribution parameters were only meant to attribute conversions. The code to update the attribution cookie even checks the config flag configConversionAttributionFirstReferrer. So why should it be used for attributing visits.
Nevertheless it looks like the code in core always checked the attribution values first when checking for a campaign for a visit.
This currently is in conflict with the MarketingCampaignsReporting plugin, as it fully ignores such values.
So if a user comes to a page with campaign A, the attribution cookie will be set to campaign A. It will also be sent as attribution value, so core will directly detect them as campaign. As the campaign paramters in the url are also present the MarketingCampaignsReporting plugin, will detect the same campaign.
If the same visitor returns with a new campaign within the same visit, this actually causes problems. The attribution cookie still contains campaign A (as the javascript tracker only updates the cookie on session start), so campaign A will be sent as attribution value, while the tracked url contains parameters for campaign B.
Core will still detect campaign A, due to the attribution values. Now the MarketingCampaignsReporting jumps in and detects campaign B from the tracked url. As this campaign now differs from the original one, the plugin forces a new visit.
An ongoing action will now no longer contain campaign B in the tracked url, but as the attribution cookie still exists, campaign A will be sent with all ongonig actions. I haven't debugged that in detail, but I guess core would in this case try to start a new visit, as the campaign differs from the last visit (forced by the plugin), somehow causing those action to be attached to the first visit...
So this bug is actually another reason for #20067

@tsteur @mattab What we actually need is a clear definition how the attribution should work. We over and over again are trying to fix certain issues in the attribution. Without knowing exactly how the whole thing should work we will most likely create new regressions each time we try to fix something...

Thanks for the extra detail @sgiehl 👍

Totally agree that a definitive specification for attribution is sorely needed. It's difficult to fix issues without knowing exactly what the correct behavior should be and thus often ends up as fixing symptoms and not causes. I think it would be a valuable piece of documentation both for developers going forward and for advanced users wanting a deeper insight into attribution data. 🚀

@sgiehl Do you think it will be realistically possible to merge and review this PR without a comprehensive specification for attribution and/or implementing #20067 ?

Is there anything else I can do to help move things along?

Personally I think this should be good to merge, once all tests have been fixed.
But we should discuss this with @mattab first I think. As generally removing the referrer detection for visits based on _rcn parameter is kind of a breaking change.

The two tests that are breaking are both when, in a single visit, a second link is followed with the same pk_campaign as the first link, but a different referring host. The tests are expecting no new visit to be created as campaign provided by the _rcn tracker parameter would be the same, but since this change now ignores the _rcn parameter for campaign detection the behaviour has changed and a new visit is created.

It's currently:

This PR:
✔️ Following a second campaign link, different from the first in the same visit will create a new visit with all subsequent actions assigned to the new visit
❌ Following a second campaign link, the same as the first but with a different referrer host will create a new visit.

vs

Existing behaviour:
❌ Following a second campaign link, different from the first in the same visit will create a new visit but all subsequent actions will be assigned to the first visit
✔️ Following a second campaign link, the same as the first but with a different referrer host will NOT create a new visit.

@sgiehl Perhaps rather than completely ignoring the _rcn parameter for campaign detection we could check to see if it is the same as the campaign url parameter (eg. pk_campaign) and if so cancel the new visit creation. Do you think this could work?

@bx80 That's something we could implement. But that makes all that stuff even more complex.
I'm actually unable to say what the expected behavior would be here. I guess the tests are using the flag to start a new visit when the website referrer changes. Should we then not create a new visit, even if the current visit was actually started with a campaign?
I would say it's correct to start a new visit in that case, as logically the visit comes from another referrer and not from the campaign anymore.

From @mattab via slack:

It sounds OK to me that a a new visit is created when following a second link in the same session with a different referrer host but the same campaign name.

However it should not create a new visit if the second referrer is an "excluded referrer" host (such as paypal.com)

I've updated the two failing referrer tests to reflect the new behaviour.
I've also added a new test test_forceNewVisit_shouldNotForceANewVisitWhenCampaignIsTheSameAndSecondReferrerIsExcluded to ensure that excluded referrers will not create a new visit.

@sgiehl Would it be possible to share the source of the attribution / referrer diagram you created? It'd be good to update it with this behaviour change.

@bx80 I had posted the charts here: #18612 (comment)
I had created them with a random online tool back then, as we didn't have a company solution yet. My links to the charts don't work anymore, so guess they where dropped by the service. We might need to recreate them.

It sounds OK to me that a a new visit is created when following a second link in the same session with a different referrer host but the same campaign name.

That actually doesn't reflect what this PR will changes. We will simply ignore campaigns sent with the attribution parameters in the future. So actually a second "visit" does not have a campaign set at all.

It sounds OK to me that a a new visit is created when following a second link in the same session with a different >>referrer host but the same campaign name.

That actually doesn't reflect what this PR will changes. We will simply ignore campaigns sent with the attribution >parameters in the future. So actually a second "visit" does not have a campaign set at all.

@sgiehl Yes, this was in relation to the two tests that specifically check that a new visit is not created when the referrer host is different but the campaign is the same. This PR changes that behaviour, hence the need to confirm that it's okay to create a new visit if the referrer is different but the campaign is the same.

So actually a second "visit" does not have a campaign set at all.

Not from the referrer attribution parameter which we now ignore, but the second visit can have a campaign via one of the campaign URL parameters, eg. pk_campaign.

I can recreate your attribution diagram in a shared tool, update it to reflect what this PR will change and then maybe that should be reviewed and should become the definitive document on how things should work?

There are still some tests failing (not having a campaign when one was expected), I'll chase those down.

@bx80 The problem is still, that we now always ignore those parameters for visit attribution. And at least according to the tracking documentation those parameters should work. See https://developer.matomo.org/api-reference/tracking-api
On the other side, the documentation on the php tracker for the method that sets those parameters sound more like it's only used for conversions: https://developer.matomo.org/api-reference/PHP-Matomo-Tracker#setattributioninfo

Looking at the attribution flow we would basically remove this parts:

This needs to be confirmed by @mattab

The change makes sense to me overall if i understand the discussion correctly. 👌

But additionally because it's a tricky topic that we ideally want documented, to really confirm @bx80 Would it be maybe possible to see the diagram after the change? (we have tools like Miro or Figma we can use btw)

As it would be a breaking change, which we should avoid for a minor release, we could implement that using a config flag for now.

I don't think it's needed to hide behind a flag in this case 👍

As mentioned in the previous comment we might need to update the documentation once this has been changed.

Which documentation do you think should be updated @sgiehl?
Otherwise the diagram will be great to have up to date as then we can answer questions confidently about how it works.

This would be the new flow for visits if the attribution parameters are ignore for visit attribution.

The documentation that would need to be updated is https://developer.matomo.org/api-reference/tracking-api
It currently mentions, that those parameters can be set to attribute the visit. And that is, what it imho makes a breaking change.

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@sgiehl If we're not going to add a config flag, then other than updating the developer docs, are you now happy for this to be merged?

@bx80 As mentioned before: from a code side of view those changes would be ready to merge. Even though it could make sense to mention in the change log that the attribution parameters can no longer be used to attribute visits.

BUT this is some sort of breaking change. So we should not include that in a patch release. We should either implement it using a config flag or at least release 4.14.0 instead of 4.13.4
This might be something @mattab needs to decide.