[WIP] Di compatibility (#4434) #4435

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
3 participants
@noopable
Contributor

noopable commented May 7, 2013

Relative to #4434
Now we cannot get some instances from the di even if the zf2's simple classes. This PR is a hotfix for it.
And this also enhances the capability to manage dependencies explicitly. But it has a minor BC break. If we should keep BC strictly, remove a5f1362 , b0d918e

Overview

METHOD_IS_REQUIRED:

Resolve dependencies
by specified params and TypePreference and name as Class.
Missing parameter and raise exception

  • constructor
  • instantiator
  • set method option METHOD_IS_REQUIRED or (required => true) explicitly

METHOD_IS_EAGER:

Resolve dependencies
by specified params and TypePreference and name as Class.
Missing parameter and give up resolving

  • AwareInterface's setter
  • set method option METHOD_IS_EAGER explicitly

METHOD_IS_OPTIONAL:

Resolve dependencies only by specified params

  • setter injection

  • Add issue reproduce test
  • Initial implementation
  • Collect feedback
  • Test coverage
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Di

This comment has been minimized.

Show comment Hide comment
@samsonasik

samsonasik May 7, 2013

Contributor

remove @Package

@samsonasik

samsonasik May 7, 2013

Contributor

remove @Package

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 7, 2013

Member

@noopable I've marked this a "work in progress" as it's clear you've written tests, but are still working on solutions.

Member

weierophinney commented May 7, 2013

@noopable I've marked this a "work in progress" as it's clear you've written tests, but are still working on solutions.

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 8, 2013

Contributor

Thanks. I'll be counting on you.

Contributor

noopable commented May 8, 2013

Thanks. I'll be counting on you.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 8, 2013

Member

@noopable Oh, I thought you were doing the work. :)

Member

weierophinney commented May 8, 2013

@noopable Oh, I thought you were doing the work. :)

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 8, 2013

Contributor

@weierophinney
Oops! I'm sorry.I didn't understand which .
I’ll give it a try.

Contributor

noopable commented May 8, 2013

@weierophinney
Oops! I'm sorry.I didn't understand which .
I’ll give it a try.

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 9, 2013

Contributor

I wrote the first implementation.
Please give me feedback on this.

Contributor

noopable commented May 9, 2013

I wrote the first implementation.
Please give me feedback on this.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable Looks great! If you're ready to sign off on it -- i.e., it solves the issues you were seeing with forms -- I can get this into 2.2.0RC3.

Member

weierophinney commented May 9, 2013

@noopable Looks great! If you're ready to sign off on it -- i.e., it solves the issues you were seeing with forms -- I can get this into 2.2.0RC3.

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 9, 2013

Contributor

@weierophinney
Thanks.
If the current state was ok, I want to release it now. And I will make tests in another PR.
If it is accepted, we can revert the commits ( 69dc3de 9afbe6f ) and some issues may be .

Contributor

noopable commented May 9, 2013

@weierophinney
Thanks.
If the current state was ok, I want to release it now. And I will make tests in another PR.
If it is accepted, we can revert the commits ( 69dc3de 9afbe6f ) and some issues may be .

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable Interestingly, I cannot revert either of those two commits, as doing so introduces new failures and/or errors.

If I revert 9afbe6f, I get new failures in the FormElementManagerFactory tests, in the testCsrfCompatibility and testCsrfWorkflow methods; essentially, I do not receive the expected hashes.

If I revert 69dc3de, I get errors in the same test suite, in the testWillInstantiateFormFromDiAbstractFactory, testNoWrapFieldName, and same CSRF tests as above. In each case, I get the exception "An abstract factory could not create an instance..."

Can you try reverting these on your branch and figuring out what changes need to be made to make them pass?

Member

weierophinney commented May 9, 2013

@noopable Interestingly, I cannot revert either of those two commits, as doing so introduces new failures and/or errors.

If I revert 9afbe6f, I get new failures in the FormElementManagerFactory tests, in the testCsrfCompatibility and testCsrfWorkflow methods; essentially, I do not receive the expected hashes.

If I revert 69dc3de, I get errors in the same test suite, in the testWillInstantiateFormFromDiAbstractFactory, testNoWrapFieldName, and same CSRF tests as above. In each case, I get the exception "An abstract factory could not create an instance..."

Can you try reverting these on your branch and figuring out what changes need to be made to make them pass?

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 9, 2013

Contributor

I'm sorry.
something is wrong about me.

Contributor

noopable commented May 9, 2013

I'm sorry.
something is wrong about me.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable revert & new commit is good -- it's explicit.

BTW, just pulled, and seeing 3 new failures on DiTest: testNewInstanceThrowsExceptionOnBasicCircularDependency, testNewInstanceThrowsExceptionOnThreeLevelCircularDependency, and testNewInstanceThrowsExceptionWhenEnteringInMiddleOfCircularDependency -- each with the message "Missing instance/object for parameter ...". My guess is it's related to 4b3ed42.

Member

weierophinney commented May 9, 2013

@noopable revert & new commit is good -- it's explicit.

BTW, just pulled, and seeing 3 new failures on DiTest: testNewInstanceThrowsExceptionOnBasicCircularDependency, testNewInstanceThrowsExceptionOnThreeLevelCircularDependency, and testNewInstanceThrowsExceptionWhenEnteringInMiddleOfCircularDependency -- each with the message "Missing instance/object for parameter ...". My guess is it's related to 4b3ed42.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable I have a working solution:

diff --git a/library/Zend/Di/Di.php b/library/Zend/Di/Di.php
index f70a457..a1fc612 100644
--- a/library/Zend/Di/Di.php
+++ b/library/Zend/Di/Di.php
@@ -12,6 +12,7 @@ namespace Zend\Di;
 use Closure;
 use ReflectionClass;
 use Zend\Di\Exception;
+use Zend\ServiceManager\Exception\ExceptionInterface as ServiceManagerException;

 /**
  * Dependency injector that can generate instances using class definitions and configured instance parameters
@@ -757,9 +758,25 @@ class Di implements DependencyInjectionInterface
                     } else {
                         $resolvedParams[$index] = $this->get($computedParams['retrieval'][$fqParamPos][0], $callTimeUserParams);
                     }
-                } catch (\Exception $e) {
+                } catch (Exception\RuntimeException $e) {
+                    if ($methodRequirementType & self::RESOLVE_STRICT) {
+                        //finally ( be aware to do at the end of flow)
+                        array_pop($this->currentDependencies);
+                        // if this item was marked strict,
+                        // plus it cannot be resolve, and no value exist, bail out
+                        throw new Exception\MissingPropertyException(sprintf(
+                            'Missing %s for parameter ' . $name . ' for ' . $class . '::' . $method,
+                            (($value[0] === null) ? 'value' : 'instance/object' )
+                        ),
+                        $e->getCode(),
+                        $e);
+                    } else {
+                        //finally ( be aware to do at the end of flow)
+                        array_pop($this->currentDependencies);
+                        return false;
+                    }
+                } catch (ServiceManagerException $e) {
                     //Zend\ServiceManager\Exception\ServiceNotCreatedException
-                    //Zend\Di\Exception\RuntimeException
                     if ($methodRequirementType & self::RESOLVE_STRICT) {
                         //finally ( be aware to do at the end of flow)
                         array_pop($this->currentDependencies);
Member

weierophinney commented May 9, 2013

@noopable I have a working solution:

diff --git a/library/Zend/Di/Di.php b/library/Zend/Di/Di.php
index f70a457..a1fc612 100644
--- a/library/Zend/Di/Di.php
+++ b/library/Zend/Di/Di.php
@@ -12,6 +12,7 @@ namespace Zend\Di;
 use Closure;
 use ReflectionClass;
 use Zend\Di\Exception;
+use Zend\ServiceManager\Exception\ExceptionInterface as ServiceManagerException;

 /**
  * Dependency injector that can generate instances using class definitions and configured instance parameters
@@ -757,9 +758,25 @@ class Di implements DependencyInjectionInterface
                     } else {
                         $resolvedParams[$index] = $this->get($computedParams['retrieval'][$fqParamPos][0], $callTimeUserParams);
                     }
-                } catch (\Exception $e) {
+                } catch (Exception\RuntimeException $e) {
+                    if ($methodRequirementType & self::RESOLVE_STRICT) {
+                        //finally ( be aware to do at the end of flow)
+                        array_pop($this->currentDependencies);
+                        // if this item was marked strict,
+                        // plus it cannot be resolve, and no value exist, bail out
+                        throw new Exception\MissingPropertyException(sprintf(
+                            'Missing %s for parameter ' . $name . ' for ' . $class . '::' . $method,
+                            (($value[0] === null) ? 'value' : 'instance/object' )
+                        ),
+                        $e->getCode(),
+                        $e);
+                    } else {
+                        //finally ( be aware to do at the end of flow)
+                        array_pop($this->currentDependencies);
+                        return false;
+                    }
+                } catch (ServiceManagerException $e) {
                     //Zend\ServiceManager\Exception\ServiceNotCreatedException
-                    //Zend\Di\Exception\RuntimeException
                     if ($methodRequirementType & self::RESOLVE_STRICT) {
                         //finally ( be aware to do at the end of flow)
                         array_pop($this->currentDependencies);
@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable The solution posted above works, and does not present a hard dependency on ServiceManager.

Member

weierophinney commented May 9, 2013

@noopable The solution posted above works, and does not present a hard dependency on ServiceManager.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable Looks good! Preparing to merge now!

Member

weierophinney commented May 9, 2013

@noopable Looks good! Preparing to merge now!

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 9, 2013

Contributor

@weierophinney
Thank you very much.
I'm sorry about bad commits.

Contributor

noopable commented May 9, 2013

@weierophinney
Thank you very much.
I'm sorry about bad commits.

weierophinney added a commit that referenced this pull request May 9, 2013

weierophinney added a commit that referenced this pull request May 9, 2013

[#4435] CS fixes
- order of use statements
- ensure line between use statements and class docblock

@ghost ghost assigned weierophinney May 9, 2013

weierophinney added a commit that referenced this pull request May 9, 2013

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney May 9, 2013

Member

@noopable No worries -- great work!

Member

weierophinney commented May 9, 2013

@noopable No worries -- great work!

@noopable

This comment has been minimized.

Show comment Hide comment
@noopable

noopable May 9, 2013

Contributor

Thank you!

Contributor

noopable commented May 9, 2013

Thank you!

@noopable noopable deleted the noopable:issue/4434 branch Dec 13, 2013

weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015

[zendframework/zendframework#4435] CS fixes
- order of use statements
- ensure line between use statements and class docblock

weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-di that referenced this pull request May 15, 2015

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