Full refactor of site content detection and no data page logic

[sgiehl]

Description:

The site detection logic as well as the no data page has grown too rapidly with random new change requests resulting in an unmaintainable, mostly untested code, having direct dependencies to certain plugins with tons of technical dept.

This PR fully refactors all the logic around site content detection as well as the full handling of the no data page.
Everything should now be easier adjustable, reusable, replaceable and extendable by plugins (without having to add changes in core)

The site content detection will the have two major parts:

• class Piwik\SiteContentDetector
  this one will handle the overall detection as it was before
• multiple classes extending Piwik\Plugins\SitesManager\SiteContentDetection\SiteContentDetectionAbstract
  Those files define the detections we provide. e.g. WordPress, Cloudflare, Osano, ...
  In additional those classes can provide methods to render additional tabs or entries for the others tab on the no data page

The no data page basically still uses the SiteContentDetector to check what was detected or not. But all detection classes might now provide the own tab content. The order of the tabs is defined by a priority each class can provide. That way we can easily define which tab should be displayed if it was detected.

fixes #21200

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

[caddoo]

I would say simplify this and have the inheritor manually define the ID, the reason is because the ID is now coupled directly to the classname, and I can see this value is used in things like cache.

It could make refactoring class names difficult in future

[michalkleiner]

Yeah, I'd agree here, the inheriting class could define the id. It can even be a public static $id property so that we can directly use it instead of calling a method (probably minor to no performance difference).

[sgiehl]

I don't like using static properties for stuff that needs to be defined/overwritten in each subclass. This makes it a lot harder for a developer to understand it, without adding comments. Abstraction is meant to be used for this purpose. So either we leave it the way it is, or we make that method abstract and have it defined in each subclass.
And the purpose of having it automatically defined based on the class name ensures that the id is unique - at least for the stuff defined in core. Non unique IDs would cause a detection to be overwritten and to become unavailable.

[michalkleiner]

Ok for me as it is, we can always iterate on it if we feel strongly about some other approach. I guess I'm just used to it as Silverstripe CMS is all based on static configs, so it's a habit for me.

[sgiehl]

I used to use that a lot in the past as well. But at some point I tried starting to use abstract methods instead, so others that need to reuse the code have it easier and can't create problems by accident without even knowing it. If there would be something like abstract attributes, that would be what I would prefer most 😆

[caddoo]

Should the return value of this be nullable? Shouldn't there always be instructions?
Same question for the $detector param

[michalkleiner]

This is in the 'Others' tab, so a detector can return content for both the tab and the Others tab, but we won't want to display in the Others if it's detected and the tab displayed, so I'd say this is correct as nullable.

[caddoo]

Is there a reason this is static? Also might be good to add a return type while you are here

[sgiehl]

Hm. It's a stateless method, that doesn't work with any instance attributes. So there is actually no reason for having it non static. Could change that, but that would also mean changing getKnownConsentManagers to non static as it uses that method. What would be your argument for having it non static?

[caddoo]

My only reason for non-static is in it's current state it can't be mocked making testing difficult, no strong opinion, up to you.

[sgiehl]

It should still be possible to simply use a full mock class whereever needed. The class is loaded using StaticContainer everywhere, so it could be replaced in testing.

[tsteur]

Hi @sgiehl , @caddoo @michalkleiner there may be some bug in there. Not sure if I'm seeing it correctly.

For a lot of tech it passes the classname to check if it was detected but the actual ID is only the last part of the name.

[sgiehl]

Passing the object id is correct. Already came across that problem when working on #21247 and fixed it in that PR.
Will push a fix on 5.x-dev directly.

[tsteur]

Thanks @sgiehl . We'll then need a new RC after the fix is applied so we can make our tests work and also test overall