-
Notifications
You must be signed in to change notification settings - Fork 25
[zend-rest] update zend-rest deps according to usage #192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
packages/zend-rest/composer.json
Outdated
| "ext-dom": "*", | ||
| "ext-reflection": "*", | ||
| "ext-simplexml": "*", | ||
| "zf1s/zend-controller": "^1.15.3", |
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.
could we keep zend-controller in suggest, please? It's required only by Zend_Rest_Controller and Zend_Rest_Route. So it's optional when only client/server classes are used.
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.
This was indeed a close call. I myself only use Zend_Rest_Controller and Zend_Rest_Route (and not the client/server classes). Both uses are indeed completely separate. suggest sounds better.
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.
@falkenhawk Can all other deps also be moved to suggest? With the exception of Zend_Exception. That makes sense when using only the MVC classes (Zend_Rest_Controller and Zend_Rest_Route).
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.
@falkenhawk I'm not sure what you meant with the eyes emoji :) I won't move the others in this PR for now.
packages/zend-rest/composer.json
Outdated
| } | ||
| }, | ||
| "suggest": { | ||
| "zf1s/zend-config": "Used in special situations or with special adapters" |
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_Config is not used at all - can be removed from suggest. Type hinting an instance of Zend_Config in Zend_Rest_Route::getInstance() does not make the class/package itself required.
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.
Just so I understand: the type hinting, if not installed, won't lead to an error?
Or you mean type hinting-only use is not enough to make it required?
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.
Both are correct
b6732a5 to
69706d6
Compare
|
@falkenhawk I think this should be fine to merge now too? |
falkenhawk
left a comment
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.
Thank you @smhg !
@falkenhawk this is the last component (at least for now). Thanks!