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.

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

Plugin updates are located within the \Updates namespace #10398

Closed
wants to merge 3 commits into from

Conversation

mattab
Copy link
Member

@mattab mattab commented Aug 15, 2016

  • in Piwik 2.x the generate:update does not work (as Piwik core will try to load the wrong class name ie. without Updates\ namespace)
  • in Piwik 3.x the generate:update command works with this pull request. plugin updates are located in the Updates\ namespace within the plugin namespace, so we read the correct namespace.

follows up commit: fda46f1

refs matomo-org/developer-documentation#139

Notes:

  • we should note in the developer changelog that plugin updates are now documented and available, so everyone knows about them.
  • this PR breaks backward compatibility in case some plugins were defining their updates without the Updates\ namespace. We should mark this in the developer changelog?
  • double check that all our own plugins define their updates in this correct namespace

@mattab mattab added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 15, 2016
@mattab mattab added this to the 3.0.0-b1 milestone Aug 15, 2016
@tsteur
Copy link
Member

tsteur commented Aug 15, 2016

We should not break API here I'd say as the updater is not so good. Eg it doesn't update all the core and the plugins at once and it may fail depending on what is updated first. It should be backwards compatible and break it with Piwik 4 so developers have more time to fix it and when development mode is enabled we can tell them to update the code. Otherwise breaks plugins like https://github.com/piwik/plugin-CustomDimensions/blob/master/Updates/0.1.2.php . If we break API here and development mode is enabled, we even have to check on every single UI request whether all the updates are defined correctly or not. Otherwise it's too risky that developers do not notice this.
We are updating from Piwik 2 to Piwik 3 so feeling bit uncomfortable breaking the update API. That's also why we kept BC with the new migration API

@mattab
Copy link
Member Author

mattab commented Aug 16, 2016

@tsteur I've added the backward compatibility layer. Tested the following use case:

  • Plugin A definines an "old style" update
  • Plugin B definines a new style update (with namespace)

-> both plugin are updated and migration SQL queries of each are executed.

// as of Piwik 3.0.0, we expect plugin updates within an Updates\ namespace
// but in Piwik 3.0.0 we stay backward compatible
// this could be @deprecated in Piwik 4.0.0
if(class_exists($backwardCompatibleClass, false)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does class_exists find the old class when autoload is disabled? Are they always first included in loadComponentsWithUpdateFile()?

Can you add a test or something to remind us to remove this in Piwik 4.0 and to make sure it does not get removed earlier? Should be added to DeprecatedMethodsTest and can be likely eg done by moving this BC code into a separate method.

@tsteur
Copy link
Member

tsteur commented Aug 16, 2016

To fix it properly it should probably also fix the filename. Fillename is eg 0.1.2.php where className is Updates_0_1_2.

@tsteur
Copy link
Member

tsteur commented Aug 16, 2016

This also needs to be mentioned in the migration guide as otherwise the plugin will fail in future versions. It also needs to explain that they cannot keep the plugin compatible with an older Piwik version as soon as they add the Updates and that they have to increase the required Piwik version in plugin.json

@mattab
Copy link
Member Author

mattab commented Aug 16, 2016

You convinced me now this is not a good idea, so closing.
Instead I propose this pull request to simply fix the issue that generated updates don't work, without any change to APIs (ideally we only change these apis when we will work on the updater): #10409

@mattab mattab closed this Aug 16, 2016
@mattab mattab deleted the plugins_updates_namespace branch October 1, 2016 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants