Inconsistencies in factory parameter resolution #431

Closed
predakanga opened this Issue Jul 15, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@predakanga
Contributor

predakanga commented Jul 15, 2016

Thanks for merging the PR on #362, but unfortunately I've just discovered a couple of bugs that it creates/reveals.

There are two big issues:

  1. If a type-hinted factory parameter is resolvable from the container, any supplied parameter will be ignored

    This is an easy fix - just need to bump AssociativeArrayResolver to the top of the ResolverChain in FactoryResolver.php

  2. FactoryParameterResolver overwrites already resolved parameters

    Again, a simple fix - just needs this preamble added (taken from other resolvers in php-di/invoker):

      $parameters = $reflection->getParameters();
    
      // Skip parameters already resolved
      if (! empty($resolvedParameters)) {
          $parameters = array_diff_key($parameters, $resolvedParameters);
      }
    

I'm happy to prepare a PR to fix these (or just provide tests for reproduction), but wanted your input: FactoryParameterResolver could be replaced with a combination of TypeHintResolver and TypeHintContainerResolver. Would you prefer a fix to FactoryParameterResolver, or removing it in favour of the two TypeHint resolvers?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 15, 2016

Member

FactoryParameterResolver could be replaced with a combination of TypeHintResolver and TypeHintContainerResolver

I reimplemented a dedicated resolver for performance reasons (see #347): the duplicated code was really nothing and factories are heavily used so it made a difference. Right now I haven't tested performances yet (will do before the stable release) but it's possible I end up "merging" AssociativeArrayResolver into FactoryParameterResolver for the same reasons. So I would suggest to keep FactoryParameterResolver as it is for now (and add the fix into it).

Thanks!

Member

mnapoli commented Jul 15, 2016

FactoryParameterResolver could be replaced with a combination of TypeHintResolver and TypeHintContainerResolver

I reimplemented a dedicated resolver for performance reasons (see #347): the duplicated code was really nothing and factories are heavily used so it made a difference. Right now I haven't tested performances yet (will do before the stable release) but it's possible I end up "merging" AssociativeArrayResolver into FactoryParameterResolver for the same reasons. So I would suggest to keep FactoryParameterResolver as it is for now (and add the fix into it).

Thanks!

@mnapoli mnapoli added the bug label Jul 15, 2016

@mnapoli mnapoli added this to the 5.4 milestone Jul 15, 2016

@predakanga

This comment has been minimized.

Show comment
Hide comment
@predakanga

predakanga Jul 15, 2016

Contributor

Good to know - I've added a note to the phpdoc for that class, so that people know the logic behind it.

PR incoming!

Contributor

predakanga commented Jul 15, 2016

Good to know - I've added a note to the phpdoc for that class, so that people know the logic behind it.

PR incoming!

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