Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removes CustomVariables plugin from core #16090

Merged
merged 25 commits into from Jul 1, 2020
Merged

Removes CustomVariables plugin from core #16090

merged 25 commits into from Jul 1, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jun 18, 2020 •
edited

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

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jun 18, 2020
@sgiehl sgiehl added this to the 4.0.0 milestone Jun 18, 2020
@sgiehl sgiehl mentioned this pull request Jun 18, 2020
Merged
@sgiehl sgiehl force-pushed the 11524-cvar branch 7 times, most recently from ed3bd8e to 30b56dc Compare June 22, 2020 12:56
@tsteur
Copy link
Member

tsteur commented Jun 22, 2020

@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 sgiehl mentioned this pull request Jun 23, 2020
Merged
@sgiehl sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 23, 2020
@sgiehl sgiehl marked this pull request as ready for review June 23, 2020 15:51
@sgiehl
Copy link
Member Author

sgiehl commented Jun 23, 2020

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

@tsteur
Copy link
Member

tsteur commented Jun 24, 2020

@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
Copy link
Member Author

sgiehl commented Jun 24, 2020

@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 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)

@tsteur
Copy link
Member

tsteur commented Jun 24, 2020

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

@sgiehl
Readds Custom Variables as submodule (#16104)
f886abf 
* Adds CustomVariables as submodule

* Revert "[TEMP] remove custom variables from system test output"

This reverts commit 30b56dc.

* use submodule branch

* test changes due to loading CustomVariables as submodule (plugin order)

* updates some expected screenshots

* improve test fixture
@tsteur
Copy link
Member

tsteur commented Jul 1, 2020

@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
Copy link
Member Author

sgiehl commented Jul 1, 2020

Will update and merge that now

@sgiehl sgiehl merged commit c6bb731 into 4.x-dev Jul 1, 2020
@sgiehl sgiehl deleted the 11524-cvar branch July 1, 2020 07:21
@mattab mattab mentioned this pull request Aug 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sgiehl @tsteur