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

Jump to bottom

Removed Piwik\Registry and replaced its usage with the container #7124

Merged
merged 6 commits into from Feb 9, 2015
Merged

Removed Piwik\Registry and replaced its usage with the container #7124

merged 6 commits into from Feb 9, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Feb 2, 2015

Piwik\Registry was very similar to using a container and only contained 2 entries.

I've replaced all that with the container. Basically here is the changelog entry:

  • The Piwik\Registry class has been removed in favor of using the container:
    • Registry::get('auth') is replaced by StaticContainer::get('Piwik\Auth')
    • Registry::set('auth', $auth) is replaced by StaticContainer::getContainer()->set('Piwik\Auth', $auth)

I have not kept BC because I have updated (for now locally) all plugins that used it. I had a look in the marketplace but couldn't find any 3rd party plugin using it (its usage was restricted to authentication).

TODO after merge:

  • update LoginAdmin
  • update LoginHttpAuth
  • update LoginLdap
  • update SSOAuth

@mnapoli mnapoli added this to the Piwik 2.11.0 milestone Feb 2, 2015
@tsteur
Copy link
Member

tsteur commented Feb 3, 2015

Registry is an API. Can we deprecate instead it for at least 2 or 3 months instead of breaking it? The class was there for a long time so there might be quite a few plugins out there who use it for whatever reasons.

If we'd break it, it would also mean people will most likely have an error at some point. They won't be able to update the plugin to a newer version as it could break Piwik if StaticContainer is not there yet in the users Piwik version. If they update Piwik first it will fail as well as the plugin still uses the Registry. They would have to update both at the same time which we currently don't do unless someone does the update including all plugins manually at once.

Also there seems to be a backslash on the left https://github.com/piwik/piwik/pull/7124/files#diff-f10b4fc6b182e822204d929ace688f74R53

@mnapoli
Copy link
Contributor Author

mnapoli commented Feb 3, 2015

I restored Registry as a bridge to the container to keep BC, I missed the @api tag on the class.

The backslash is unrelated to the change (and is in a comment so doesn't affect code) I'll leave it there to keep the diff clean.

@mnapoli
Copy link
Contributor Author

mnapoli commented Feb 9, 2015

Merged with master.

mattab pushed a commit that referenced this pull request Feb 9, 2015
Merge pull request #7124 from piwik/removed-registry
e954f84 
Removed Piwik\Registry and replaced its usage with the container
@mattab mattab merged commit e954f84 into master Feb 9, 2015
@mattab mattab deleted the removed-registry branch February 9, 2015 03:31
@tsteur
Copy link
Member

tsteur commented Feb 9, 2015

The changelog entry says the Registry has been removed but it is only deprecated and still there

@mnapoli
Copy link
Contributor Author

mnapoli commented Feb 9, 2015

Fixed

mnapoli added a commit to matomo-org/plugin-LoginLdap that referenced this pull request Feb 16, 2015
@mnapoli
refs matomo-org/matomo#7124 Updated the usage of the deprecated registry
b107dac
mnapoli added a commit to matomo-org/plugin-LoginHttpAuth that referenced this pull request Feb 16, 2015
@mnapoli
refs matomo-org/matomo#7124 Updated the usage of the deprecated registry
e1c3589
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
None yet
Projects
None yet
Milestone
Piwik 2.11.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mnapoli @tsteur @mattab