Skip to content
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

[zend-view] update zend-view deps according to usage #178

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

smhg
Copy link
Contributor

@smhg smhg commented Dec 17, 2023

I've listed everything used in view helpers under suggest.

That means a lot of additions, but also that Zend_Controller and Zend_Locale are no longer required dependencies. I can't imagine both are not installed explicitly when needed, but feel free to disagree of course.

closes #176

@falkenhawk
Copy link
Member

@smhg maybe zend-controller was listed in required dependencies, because it's tightly coupled with Zend_View_Helper_Action which tends to be used a lot? 🤔
But on the other hand, to make it in line with other view helpers, all dependencies used by them should be optional 🤔 And that is what you did. The action helper doesn't deserve a special treatment, does it?

@falkenhawk falkenhawk changed the title update zend-view deps according to usage (closes #176) [zend-view] update zend-view deps according to usage Jan 3, 2024
@smhg
Copy link
Contributor Author

smhg commented Jan 3, 2024

@falkenhawk

FWIW: in my project Zend_View is used for email template rendering, independent of Zend_Controller. It is probably not common usage.

But I do think coupling should be on the loose side. Because in the end, projects should likely be refactoring away from ZF1 and thus it is useful to require only the absolute minimum rather than the safe maximum.

And, when using zf1s as a zf1 replacement, what you are looking for includes 'less coupling' probably?

Also, specifically in the Zend_Controller case, I think it must be quite uncommon to only specify Zend_View in your project and count on it including Zend_Controller, no?

Feel free to disagree of course.

Copy link
Member

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

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

👍

@falkenhawk falkenhawk merged commit f20b922 into zf1s:master Jan 4, 2024
12 checks passed
@smhg smhg deleted the fix-zend-view-deps branch January 4, 2024 11:21
@falkenhawk falkenhawk mentioned this pull request Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zend_Locale doesn't seem to be a necessary dep of Zend_View
2 participants