@sgiehl opened this Pull Request on June 18th 2020 Member

This PR aims to remove the CustomVariables plugins from core. In a later PR we will readd it as submodule. But as it will no longer be bundled with core, we need to ensure Matomo 4 runs correctly with the plugin removed (completely)

Note: There are still a lot hard requirements for CustomVariables in core. I will try to remove them if they are not needed or move them to the new plugin repo step by step until the tests are running again.

@tsteur Shall we move the custom variables part of piwik.js to the plugin as well? Or should that remain in core?

This PR needs to be reviewed together with #16104

refs #11524

@tsteur commented on June 22nd 2020 Member

@sgiehl lets keep the tracking code in piwik.js for now and create an issue to move it to the plugin in Matomo 5. This should ensure we keep the update to Matomo 4 easier in case some people don't have the matomo.js writable etc.

@sgiehl commented on June 23rd 2020 Member

Note: There are still some parts left in core (like tracking or configuring custom variables in tracking code generator)

@tsteur commented on June 24th 2020 Member

@sgiehl looks generally good to merge once the tests pass (they currently don't by the looks).

  • BTW in tracker model can maybe remove this:
    image

  • Also in CoreAdminHome/Controller::trackingCodeGenerator() the custom variables class is still accessed without checking if plugin is activated? (for maxCustomVariables). Getting eg this error on tracking code page: Class 'Piwik\Plugins\CustomVariables\CustomVariables' not found in /Users/thomassteur/Development/piwik/plugins/CoreAdminHome/Controller.php line 221

  • In Goals VisitorDetails is a use statement that can be removed use Piwik\Plugins\CustomVariables\CustomVariables;

  • plugins/API/SegmentMetadata.php the sortSegments() uses a translation key for customvariables but seems that's fine and won't need changing as it wouldn't be set anyway if it's not activated...
@sgiehl commented on June 24th 2020 Member

@tsteur I've applied your feedback. Most tests should work correctly now. Currently failing are: Some submodule plugin tests, a mysqli fail (see #16108) and a lot of UI tests, as they changed a lot due to the removed plugin.

How shall we proceed? Should I update all tests, so we can merge this PR and later include the plugin as submodule plugin, which is disabled by default in tests? Or we merge #16104 into this one (after it and https://github.com/matomo-org/plugin-CustomVariables/pull/1 was reviewed), which should fix all failing tests, but would have the plugin as enabled in tests (like Bandwidth and others)

@tsteur commented on June 24th 2020 Member

Or we merge #16104 into this one (after it and matomo-org/plugin-CustomVariables#1 was reviewed), which should fix all failing tests, but would have the plugin as enabled in tests (like Bandwidth and others)

sounds good

@tsteur commented on July 1st 2020 Member

@sgiehl one more fixture to update otherwise good to merge 👍

I suppose this test failure is unrelated https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/41362/UIIntegrationTest_email_reports_editor.png

@sgiehl commented on July 1st 2020 Member

Will update and merge that now

This Pull Request was closed on July 1st 2020
Powered by GitHub Issue Mirror