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

Refactoring of the Dependency Injection component (v3) #6

Merged
merged 27 commits into from Nov 14, 2017

Conversation

Projects
None yet
3 participants
@tux-rampage
Contributor

tux-rampage commented Dec 11, 2015

ZF3 Refactoring Proposal

With the version switch to 3 it's a good time to refactor
the Di component with possible BC breaks (much like the proposed ServiceManager refactoring).

TODO's

  • Refactoring, code cleanup (wip)
  • Utilize Psr\Container\ContainerInterface to be compatible/utilize with ServiceManager and any other one that implements it (Follow PHP-FIG Standards)
  • Code generator for performance optimization
  • Optional ServiceManager integration (loose coupling)
  • Test cases
  • Update the documentation
  • Add Module class

TODO's (Requested Changes)

  • Since this will be a new major version, bump the minimum supported PHP version to 7.1. (This is something we decided earlier this year.) That means you can also bump the PHPUnit version to the v6 series.
  • Typehint against psr-container instead of container-interop. This will still work with zend-servicemanager; it should use versions of container-interop that extend the psr-container interfaces, which would allow using the two together.
  • Use zendframework/zend-coding-standard for checking/fixing coding standards, instead of php-cs-fixer.
  • Remove HHVM support.
  • s/instanciator/instantiator/g throughout.
  • Write up some migration documentation: how would a consumer of the v2 series migrate to v3? What are potential pitfalls?
  • Keep the v2 documentation, but potentially move it to a "legacy documentation" section, so folks still on v2 can find information.

Goals

Code cleanup

  • The ZF2 code has duplications (namingly the compiler and runtime definition)
  • The resolver code is uneccessary complex and should be put into a separate implementation
  • The InstanceManager should bereplaced by the service locator interface
  • The service locator should be exchangible (by any Interop\ContainerInterface)

Simplify

Make this component more leightweight by removing too complex scenarios like getter or property injections. Constructor-Injection is much easier to implement and debug and lowers the risk potential issues like cyclic dependencies.

Fore more complex scenarios there is always zend-servicemanager which should be used.

Add support for generating an optimized di container

This should generate a container and injectors by utilizing the
known class definitions and instance configuration.
The DependencyInjector would then at first attempt to use the generated
injector for the particular type and fall back to the default routines.

Refactor ServiceLocator and ServiceLocatorInterface

The current ones are most likely never used in the real world or by Di.
This should be integrated better and be used for looking up instances.

Make it a ZF3 Component

This component should easily integrate by using the zend-component-installer or by adding it as a zend-mvc module to the application config. This approach
will make it possible to remove dependencies from mvc or other components and to deprecate the integration package.

So people can decide whether they stay with ServiceManager only or combine it with Zend\Di.

Better integration with ServiceManager

The ZF2 integration of Di into the ServiceManager/Mvc is using an odd factory
that lacks the capability to inject already configured services into deeper
nested dependencies which makes this integration quite useless.

Example:

  • Class Foo
    • Depends on Zend\Db\Adapter\AdapterInterface
    • Depends on Bar
      • Depends on Zend\Authentication\AuthenticationServiceInterface

Assuming the service manager has configured the Zend classes as services:

While the DB Adapter for Foo is taken from the ServiceManager, the auth service
dependency for Bar is attempted to be created purely via Di. All reference to the
ServiceManager is lost.

Since the ServiceManager in ZF3 now uses Interop\ContainerInterface, we can pass the ServiceManager
for retrieving the instances the dependency injector needs.

When Using the Module approach above, Di can register an abstract service factory to handle class instanciation.

Change the parameters array role

While the concept of providing complex parameters to a service locator (or its factories) is
fine for the ServiceManager, but it's questionable for dependency injection.

Mapping a parameter hash is not only a pain to resolve (especially for nested dependencies),
they also make things uneccessarily complicated.

To remove this complexity and allow faster instanciation the provided parameters array will only be used for
the instanciation method of the requested class. It will not be passed to any other methods and/or newly instanciated dependencies.

@tux-rampage

This comment has been minimized.

Show comment
Hide comment
@tux-rampage

tux-rampage Nov 4, 2017

Contributor

Also Includes #20 (cherry picked)

Contributor

tux-rampage commented Nov 4, 2017

Also Includes #20 (cherry picked)

@tux-rampage tux-rampage changed the title from [WIP] Refactoring of the Dependency Injection component to Refactoring of the Dependency Injection component Nov 4, 2017

@tux-rampage tux-rampage changed the title from Refactoring of the Dependency Injection component to Refactoring of the Dependency Injection component (v3) Nov 4, 2017

Remove ignore-platform-reqs
This causes php 5.6 to fail with latest deps.
@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Nov 6, 2017

Member

Awesome pull request - I love the fact that you also included documentation!

I'll try and review this week so we can get a release out. When we do, we'll likely need a new version of zend-servicemanager-di as well, which I'll need your help with to ensure correctness.

Once those are in place, would you be open to maintaining these two components?

Member

weierophinney commented Nov 6, 2017

Awesome pull request - I love the fact that you also included documentation!

I'll try and review this week so we can get a release out. When we do, we'll likely need a new version of zend-servicemanager-di as well, which I'll need your help with to ensure correctness.

Once those are in place, would you be open to maintaining these two components?

@tux-rampage

This comment has been minimized.

Show comment
Hide comment
@tux-rampage

tux-rampage Nov 6, 2017

Contributor

Thank you. I created it with PSR-11 and full zend-servicemanager compatibility in mind, so zend-servicemanager-di would not be a requirement anymore. But the service factory and module/config provider could be moved to zend-servicemanager-di if it fits better there, of course.

I'd love to take the opportunity to maintain these components.

Contributor

tux-rampage commented Nov 6, 2017

Thank you. I created it with PSR-11 and full zend-servicemanager compatibility in mind, so zend-servicemanager-di would not be a requirement anymore. But the service factory and module/config provider could be moved to zend-servicemanager-di if it fits better there, of course.

I'd love to take the opportunity to maintain these components.

@weierophinney

Here's my initial feedback:

  • Since this will be a new major version, bump the minimum supported PHP version to 7.1. (This is something we decided earlier this year.) That means you can also bump the PHPUnit version to the v6 series.
  • Typehint against psr-container instead of container-interop. This will still work with zend-servicemanager; it should use versions of container-interop that extend the psr-container interfaces, which would allow using the two together.
  • Use zendframework/zend-coding-standard for checking/fixing coding standards, instead of php-cs-fixer.
  • Remove HHVM support.
  • s/instanciator/instantiator/g throughout.
  • Write up some migration documentation: how would a consumer of the v2 series migrate to v3? What are potential pitfalls?
  • Keep the v2 documentation, but potentially move it to a "legacy documentation" section, so folks still on v2 can find information.

I think with these latter two items in place, in particular, I can better review the impact, and users will have a better experience in evaluating upgrades to the new version.

Thanks a ton for all the work you've done so far! A net loss of lines of code is ALWAYS welcome!

@tux-rampage

This comment has been minimized.

Show comment
Hide comment
@tux-rampage

tux-rampage Nov 8, 2017

Contributor

Thanks for your feedback, I will change it accordingly.
Since support for php < 7.1 is dropped, I'll make use of the 7.1 feature set, including return and scalar typehints.

About the PSR container hints, do you want me to replace them with Interop\Container or leave it as is?

Contributor

tux-rampage commented Nov 8, 2017

Thanks for your feedback, I will change it accordingly.
Since support for php < 7.1 is dropped, I'll make use of the 7.1 feature set, including return and scalar typehints.

About the PSR container hints, do you want me to replace them with Interop\Container or leave it as is?

tux-rampage added some commits Nov 9, 2017

Add descriptive comment
container-interop/container-interop is superseded by psr/container which
is used all through zend-di. There is still a string literal reference
to container-interop for backwards compatibility when a
container-interop instance is requested.

tux-rampage added some commits Nov 14, 2017

Fix style violations
Switching to zendframework/zend-coding-standard reported several style
violations. This commits addresses these violations
@tux-rampage

This comment has been minimized.

Show comment
Hide comment
@tux-rampage

tux-rampage Nov 14, 2017

Contributor

@weierophinney I have made the requested changes.

Contributor

tux-rampage commented Nov 14, 2017

@weierophinney I have made the requested changes.

@weierophinney weierophinney merged commit caf46ff into zendframework:develop Nov 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on develop at 59.422%
Details

weierophinney added a commit that referenced this pull request Nov 14, 2017

Merge pull request #6 from tux-rampage/zf3
Refactoring of the Dependency Injection component (v3)

Conflicts:
	.travis.yml
	composer.json
	composer.lock
	phpcs.xml
	src/Config.php
	src/Definition/ArrayDefinition.php
	src/Definition/BuilderDefinition.php
	src/Definition/ClassDefinition.php
	src/Definition/CompilerDefinition.php
	src/Definition/RuntimeDefinition.php
	src/DefinitionList.php
	src/Di.php
	src/Exception/UnexpectedValueException.php
	src/InstanceManager.php
	src/ServiceLocator.php
	src/ServiceLocator/DependencyInjectorProxy.php
	src/ServiceLocator/Generator.php
	test/ConfigTest.php
	test/Definition/CompilerDefinitionTest.php
	test/Definition/RuntimeDefinitionTest.php
	test/DiCompatibilityTest.php
	test/DiTest.php
	test/InstanceManagerTest.php
	test/TestAsset/AggregateClasses/AggregateItems.php
	test/TestAsset/CallbackClasses/B.php
	test/TestAsset/ConstructorInjection/C.php
	test/TestAsset/ConstructorInjection/OptionalParameters.php
	test/TestAsset/ContainerExtension.php
	test/TestAsset/DependencyTree/Level2.php
	test/TestAsset/InjectionClasses/A.php
	test/TestAsset/IterableDependency.php
	test/TestAsset/StaticFactory.php
	test/_files/definition-array.php
	test/_files/sample.php

weierophinney added a commit that referenced this pull request Nov 14, 2017

@tux-rampage tux-rampage deleted the tux-rampage:zf3 branch Nov 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment