-
Notifications
You must be signed in to change notification settings - Fork 28
Allow using v3 releases of zend-stdlib and zend-eventmanager #42
Conversation
The APIs consumed from each are identical between versions, allowing the component to target either.
Since this patch is specifying two different series of releases, we need to ensure we test against them all. As such, this introduces: - composer.lock file - composer scripts for testing, CS, and coverage uploads - travis configuration to test each PHP version against lowest, locked, and latest.
Now that these have run against an LDAP server, I see that either tests need to change, or code needs to change, to allow zend-eventmanager v3 to work. I'm firing up vagrant locally so I can test and resolve. |
Before you put too much effort into this: I wanted to tag 2.7 tomorrow which includes the removal of the Zend\StdLib as requirement and uses it only for testing purposes! So perhaps you might want to wait for that (or code against the develop-branch which has that already included!) Thanks for the PR though as I would have done it otherwise over the weekend! |
@heiglandreas Oh! Well, that's good, then! Still need to allow zend-eventmanager v3, though, and I'll get that working in here, and then create a new PR against develop with those changes. Thanks for the heads-up! |
.*.sw* | ||
.*.un~ | ||
clover.xml | ||
composer.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? All builds on Travis-CI use Composer update so what's the point of this file when is not involved in testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Maks3w Please see the changes in .travis.yml
, which has been updated to use the lockfile so we can use the lowest / locked / latest strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- if [[ $DEPS == 'latest' ]]; then travis_retry composer update $COMPOSER_ARGS ; fi
- if [[ $DEPS == 'lowest' ]]; then travis_retry composer update --prefer-lowest --prefer-stable $COMPOSER_ARGS ; fi`
Both scenarios composer.lock is ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and in line 82 it's used. those two lines only test max and min version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that composer. lock is not the provided. Its the generated by composer update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only when $DEPS is either 'latest' or 'lowest' the composer.lock
is overwritten. When $DEPS is either 'locked' or not set at all the original composer.lock
will be used. So no problem from my POV. Or am I missing something completely out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ager v3 Updates the `Node` class to marshal the EventManager instance in a cross-version compatible fashion.
Testing against the lowest allowed exposes an issue found on other repositories with how PHPUnit reports its version. Since the test bootstrap is no longer necessary (it essentially only ensures autoloading is in place), it can be removed.
@heiglandreas Okay, last change appears to have things running. I'll create a new PR shortly against the develop branch that incorporates the travis and zend-eventmanager related changes. |
* Otherwise, marshal the instance in a version-agnostic way, and return | ||
* it. | ||
* | ||
* @return null\EventManager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be a pipe instead of a backslash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, yes it should. Correcting for #43.
Closing in favor of #43. |
This patch updates the dependency constraints as follows:
^5.5 || ^7.0
, putting it in line with other components. That said, I'm aware of PHP 7 differences that currently mean tests do not pass (due to changes in sorting behavior).^2.7 || ^3.0
^2.6 || ^3.0
These latter two work, due to the fact that the APIs consumed have not changed between releases.
To ensure that the component works against each set of releases, this patch also updates the travis configuration to test against lowest supported verions, a lockfile, and latest versions.