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

Require zend-stdlib #22

Closed
wants to merge 1 commit into from
Closed

Require zend-stdlib #22

wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Contributor

zend-eventmanager used to "require" zend-stdlib but in 3.0 (branch develop) this is gone (zend-stdlib is moved from require to require-dev).

This means that zend-stdlib was always installed when zend-code was.
But by allowing eventmanager ^3.0, this is not true anymore, and breaks packages that relied on this assumption (see e.g. #21 and Ocramius/ProxyManager#260)

@Maks3w
Copy link
Member

Maks3w commented Nov 16, 2015

👎 StdLib is used only in 1 place and does not justify add it as a default dependency.

Instead I suggest refactor the code for not depend of StdLib

Related:
zendframework/zendframework#4238
zendframework/zendframework#4238 (comment)

@Maks3w Maks3w added the invalid label Nov 16, 2015
@stof
Copy link
Contributor

stof commented Nov 16, 2015

@Maks3w for now, it is required for the library to work, so it should be declared as a dependency as a quick fix to make the library work again without a fatal error.
Refactoring the code to eliminate the dependency is indeed a good idea, but it makes this PR invalid only if you can do this refactoring as fast as you can merge this PR, to fix the issue for people using your package

@Maks3w
Copy link
Member

Maks3w commented Nov 16, 2015

The point is where is the limit between "suggest" dependencies and "require" dependencies.

/cc @weierophinney

@stof
Copy link
Contributor

stof commented Nov 16, 2015

@Maks3w there is a fatal error when trying to use zend-code without having zend-stdlib, so it does not seem optional, even though it is used in a single place. Especially given that it was not happening when using only 2.x components (because zend-stdlib was a transitive dependency).

And from what I see, it would be very hard to use zend-code without triggering any code path relying on the ValueGenerator (as the place using zend-stdlib is the ValueGenerator constructor).

@weierophinney
Copy link
Member

The proposed solution is wrong, because it's based on a faulty analysis of the root cause.

The better solution is to change the ValueGenerator to depend on simply ArrasyObject, and not the zend-stdlib variant. That class is a polyfill, and unnecessary starting with 5.5. With that change, we no longer need to reintroduce the dependency.

Update this PR to do that instead, and we can accept it.

@weierophinney
Copy link
Member

@stof one other note: @Maks3w is correct. There are many paths of usage of this component that do not require stdlib: e.g. using only the reflection subcomponent, or using only the annotation subcomponent. Only usage of the generator requires it, and that's an optional use case; as such, the dependency was correctly marked as optional. That said, with my suggested change, it won't be required at all.

@nicolas-grekas
Copy link
Contributor Author

@weierophinney PR updated, is that what you had in mind?

@Maks3w
Copy link
Member

Maks3w commented Nov 16, 2015

Close but not enough. Give me a few minutes while end to write a BC safe alternative

@@ -18,14 +18,12 @@
},
"require-dev": {
"doctrine/common": ">=2.1",
"zendframework/zend-stdlib": "~2.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is still necessary. The testsuite relies on the stdlib error handler in one place (currently not failing because of zend-version depending on it transitively and #10 is not yet merged)

@nicolas-grekas nicolas-grekas deleted the fix-dep branch November 18, 2015 08:40
weierophinney added a commit that referenced this pull request Nov 18, 2015
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.

4 participants