-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Added "zendframework/zend-filter" into suggest at Zend\Stdlib #6667
Conversation
@@ -14,17 +14,17 @@ | |||
}, | |||
"target-dir": "Zend/Stdlib", | |||
"require": { | |||
"php": ">=5.3.23" | |||
"php": ">=5.3.23", | |||
"zendframework/zend-filter": "self.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.
zend-filter should be on 'sugggest', only used by UnderscoreNamingStrategy
Just to understand it, the library without zend-filter and zend-servicemanager won't works (throws class not found exceptions) in which way they should be suggested? They are required imho. Did you agree? |
}, | ||
"require-dev": { | ||
"zendframework/zend-eventmanager": "self.version", | ||
"zendframework/zend-serializer": "self.version", | ||
"zendframework/zend-servicemanager": "self.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.
zendframework/zend-servicemanager is right for require-dev
that depends on class you're using like I proposed comment before. |
Sorry that i reply i don't want to insist but i want only better understand your vision. I create a gist about this case here: https://gist.github.com/wdalmut/445e89b478a349ba0705 If you test it:
It won't works because of missing filter library. Just add it and update the dependencies list and run it again, it won't works because missing service manager library, i'm not trying to use any particular thing, just the hydrator class method that of course need the service manager because of it internal coding, but just because of its internals those libraries should be included. Can you better explain why those dependencies should not required in your opinion? Probably i miss something, i'm not a big follower of zf. |
I personally think that just because this required in one class file/two, it doesn't mean it must be added in 'require' section for the component that consists of many classes. It should be only in 'suggest' part, maybe add a description that it to support usage of what class. |
Get that but i don't agree with it, in my opinion those are required not suggested because i'm not use filters or the service manager i am using an hydrator and effectively i can't because of missing libraries. |
Hydrators are only a small part of Stdlib. If you only want to use e.g. the ArrayUtils, you don't care about ServiceManager or Filter. |
Yep, i know that, but ClassMethods is still a part of the stdlib library that is sold as a self contained project, in my opinion it is enough to support all features and not only a big set of it. Hydrators are coupled with service manager and filters because of ClassMethods strategy. |
Same reflection for Zend\MVC #6587 |
These should be "suggest" entries, not "require" entries. As @DASPRiD correctly notes, hydrators are not the only reason to use "zendframework/zend-filter": "To support hydrator filters" (We already do this to denote that the service manager is optional to this component.) Ideally, we'll move hydrators to their own component in the future. But the "suggest" entries are the correct place to denote optional dependencies and/or dependencies that are only for specific features of a component. |
Hi and thanks to all that discuss with me about this, my expectations about this pull request are in general wrapped in a single sentence: @weierophinney Ideally, we'll move hydrators to their own component in the future Mainly because as a user of the Stdlib package, when i use:
It should works by itself but effectively the library fails because 2 dependencies are missing by default (filter and service manager), so for that reason i suggest to include as required and not suggested. Thanks again to everyone i really appreciate all discussion about this topic. Bests |
@wdalmut My point is this: I'm not going to merge unless you make them suggested dependencies only. Yes, if you're using just hydrators, that means you need to add more than just zend-stdlib to your list of requirements. However, this is the correct way to deal with optional functionality when defining a package's requirements. Hydrators are opt-in, and not the only use case for the component. I understand that it would be easier for you, and for those using just the hydrators from zend-stdlib, to be able to just add zend-stdlib to your composer.json. However, due to the reasons stated already, that is not a viable option. |
I already solved my problem adding missing requirements, you can close this pull because doesn't fit the zendframework general overview of the stdlib library. My point is not only to solve an issue (that is just a couple of rows in a json file) because i never see hydrators as opt-in features of the zf stdlib. Just close it, i'm happy with your reply. Thanks again |
just move "zendframework/zend-filter" into suggest, remove the "zendframework/zend-servicemanager" from require, "rollback the "zendframework/zend-servicemanager" into "suggest" and "require-dev" and change the PR title with add "zendframework/zend-filter" into suggest at Zend\Stdlib and I think that would be ok |
Ok |
@samsonasik done! |
…pendency' into develop Close #6667
…ip-suggested-zend-filter-dependency' into develop Close zendframework/zendframework#6667
In order to use the hydrator library in Zend/Stdlib
you have to add also "zendframework/zend-filter" and
"zendframework/zend-servicemanager" otherwise a couple
of exceptions are thrown.