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

Introduced the DI container #6555

Merged
merged 11 commits into from Nov 4, 2014
Merged

Introduced the DI container #6555

merged 11 commits into from Nov 4, 2014
+420 −7

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Oct 29, 2014

I've added the DI container. I've kept things simple for now to have a readable diff.

  • require PHP-DI v5.0-dev
  • added an empty config/global.php config file, alongside the existing config files
  • the container will also include a 'config/config.php' file if it exists, this file being in .gitignore (so users can override the global config)
  • added Piwik\StaticContainer, which returns a singleton of the container: this is deprecated already but necessary so that the container can be used in singletons to migrate to DI
  • the FrontController now uses the container to create controllers

That means that we have DI in controllers.

@mnapoli mnapoli added the c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. label Oct 29, 2014
@mnapoli mnapoli self-assigned this Oct 29, 2014
@mattab
Copy link
Member

mattab commented Oct 30, 2014

Feedback:

  • I like the annotation way to inject dependencies, it is lean & readable
  • There are many dependencies in php-di - would be great to remove as many as possible when possible

Excellent to see we're getting started on DI!

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Oct 30, 2014
@mattab mattab added this to the Piwik 2.10.0 milestone Oct 30, 2014
@mnapoli
Copy link
Contributor Author

mnapoli commented Oct 30, 2014

Regarding the number of dependencies, I had planned to make ocramius/proxy-manager optional in v5.0. I've created an issue for this: PHP-DI/PHP-DI#198

I've made the change and created a new 5.0 branch. We will be using that branch in Piwik, that will allow us to make agile changes for v5.0 while not having to rush releasing that new version. I'll keep it as stable and merge features in the branch only when they are considered stable.

For those interested in following v5.0 here is the branch: PHP-DI/PHP-DI#200

@mnapoli
Copy link
Contributor Author

mnapoli commented Nov 4, 2014

@mattab this PR can be merged to be released in a beta.

The container is used to create all controllers, i.e. new $controllerClass() has been replaced by $container->make($controllerClass).
But nothing is injected anywhere, I've removed the examples using annotations.

That simple change will let us test that everything works correctly for users.

@mattab
Copy link
Member

mattab commented Nov 4, 2014

Excellent! now let's see if beta testers notice any problem or error 👍

mattab pushed a commit that referenced this pull request Nov 4, 2014
Merge pull request #6555 from piwik/php-di
96f4d53 
Introduced the DI container
@mattab mattab merged commit 96f4d53 into master Nov 4, 2014
@mattab
Copy link
Member

mattab commented Nov 4, 2014

@diosmosis the day has come that PHP-DI is included in Piwik! REJOICE!

@mnapoli mnapoli deleted the php-di branch November 4, 2014 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mnapoli mnapoli

Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mnapoli @mattab