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

Added "zendframework/zend-filter" into suggest at Zend\Stdlib #6667

Closed
wants to merge 2 commits into from

Conversation

wdalmut
Copy link
Contributor

@wdalmut wdalmut commented Sep 13, 2014

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.

@@ -14,17 +14,17 @@
},
"target-dir": "Zend/Stdlib",
"require": {
"php": ">=5.3.23"
"php": ">=5.3.23",
"zendframework/zend-filter": "self.version",
Copy link
Contributor

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

@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 13, 2014

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"
Copy link
Contributor

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

@samsonasik
Copy link
Contributor

that depends on class you're using like I proposed comment before.

@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 13, 2014

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:

vendor/bin/phpunit HydratorTest.php

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.

@samsonasik
Copy link
Contributor

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.

@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 14, 2014

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.

@DASPRiD
Copy link
Member

DASPRiD commented Sep 14, 2014

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.

@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 14, 2014

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.

@gianarb
Copy link
Contributor

gianarb commented Sep 14, 2014

Same reflection for Zend\MVC #6587

@weierophinney
Copy link
Member

These should be "suggest" entries, not "require" entries. As @DASPRiD correctly notes, hydrators are not the only reason to use Zend\Stdlib, and if you are not using the hydrators, there's no reason to install filters, the service manager, etc. If you are -- the "suggest" entries will be displayed when you run composer, and should be worded with something similar to:

"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.

@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 23, 2014

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:

$hydrator = new Zend\Stdlib\Hydrator\ClassMethods();
$clazz = new Hydratable;
$hydrator->hydrate(["me" => "data"], $clazz);
$this->assertEquals("data", $clazz->getMe());

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
Walter

@weierophinney
Copy link
Member

@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.

@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 23, 2014

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

@samsonasik
Copy link
Contributor

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

@gianarb gianarb mentioned this pull request Sep 23, 2014
@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 23, 2014

Ok

@wdalmut wdalmut changed the title [BUGFIX] missing requires Zend/Stdlib via composer Added "zendframework/zend-filter" into suggest at Zend\Stdlib Sep 23, 2014
@wdalmut
Copy link
Contributor Author

wdalmut commented Sep 23, 2014

@samsonasik done!

Ocramius added a commit that referenced this pull request Nov 22, 2014
@Ocramius
Copy link
Member

@wdalmut merged into develop at 2d2f03b, thanks!

@Ocramius Ocramius closed this Nov 22, 2014
gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants