Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Ensure that the default initializers register last #65

Merged
merged 7 commits into from
Apr 28, 2016

Conversation

weierophinney
Copy link
Member

Per #58 and #64, 2.7.0 introduced a regression due to the fact that the default initializers, injectFactory() and callElementInit(), were registered prior to delegating to the parent constructor instead of afterwards. This led to problems when the order of initializers was important (e.g., if an initializer was practicing setter injection, and the injected instance was expected when init() was called by callElementInit()).

This patch provides a test that asserts the expected behavior, and updates the FormElementManager constructor to properly inject the initializers after calling the parent constructor.

Per zendframework#58 and zendframework#64, 2.7.0 introduced a regression due to the fact that the
default initializers, `injectFactory()` and `callElementInit()`, were
registered *prior* to delegating to the parent constructor instead of
*afterwards*. This led to problems when the order of initializers was
important (e.g., if an initializer was practicing setter injection, and
the injected instance was expected when `init()` was called by
`callElementInit()`).

This patch provides a test that asserts the expected behavior, and
updates the `FormElementManager` constructor to properly inject the
initializers after calling the parent constructor.
Will simplify testing both locally and on Travis CI.
It can be safely replaced with the composer autoloader; right now, it prevents
testing against the lowest allowed dependencies.
- Need PHPUnit 4.8+ for prophecy.
@weierophinney
Copy link
Member Author

After seeing the failing tests (all against v2 of zend-servicemanager), I did some digging.

Interestingly, in the v2 releases, ServiceManager::addInitializer() defines an optional, second parameter, $topOfStack = true. Internally, it uses that to determine if the initializer should be added to the start of the list (default) or end.

Prior to v2.7, FormElementManager did the following:

$this->addInitializer([$this, 'injectFactory']);
$this->addInitializer([$this, 'callElementInit'], false);

In other words, it pushes the injectFactory() initializer to the top of the stack, but the callElementInit() factory to the end.

Updating the patch now to reflect this.

While initializer order should not be depended on, `FormElementManager`
*does*, as `init()` needs to be fired at the very last, and the form
factor needs to injected at the very beginning. This patch ensures that
behavior across both v2 and v3 versions of zend-servicemanager.
Cache the vendor directory between runs; speeds up installs.
- CS_CHECK=true
- php: 5.5
env:
- DEPS=latest
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to a lowest/locked/latest strategy, as it makes it far easier both on Travis and locally to switch between supported versions in order to test. (Which was critical for this patch, as I needed to verify behavior between zend-servicemanager major versions.)

@weierophinney weierophinney added this to the 2.8.2 milestone Apr 28, 2016
@weierophinney weierophinney self-assigned this Apr 28, 2016
When xdebug is enabled, tests are consistently taking greater than 10 minutes on
that job, causing travis to suspend the job.
weierophinney added a commit that referenced this pull request Apr 28, 2016
weierophinney added a commit that referenced this pull request Apr 28, 2016
@weierophinney weierophinney merged commit 354f7ad into zendframework:master Apr 28, 2016
weierophinney added a commit that referenced this pull request Apr 28, 2016
@weierophinney weierophinney deleted the hotfix/initializers branch April 28, 2016 22:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant