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-config] update zend-config deps according to usage #187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

smhg
Copy link
Contributor

@smhg smhg commented Jan 9, 2024

@falkenhawk brings Zend_Config deps up to date, detatching Zend_Xml.

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.

Thanks for preparing the PR!

I need to reject it, it's very subjective but we use xml config format across all our projects. Removing zend-xml from package dependencies will make it crash after update if we don't remember to add zend-xml to project dependencies, as Zend_Config uses Zend_Xml_Security right in the constructor, so it's tightly coupled.

I hope you'll understand @smhg - could you perhaps modify the PR, leaving zend-xml where it was, and only adding zend-json to suggest, please?

@falkenhawk falkenhawk changed the title update zend-config deps according to usage [zend-config] update zend-config deps according to usage Feb 25, 2024
@smhg
Copy link
Contributor Author

smhg commented Feb 25, 2024

@falkenhawk Thank you for all your work!

I totally understand if you have to be pragmatic and can't justify this change in your own projects.
In that case, I think we need to add Zend_Json, Zend_Xml and Zend_Yaml as required deps.

As a last attempt: in my project I only use Zend_Config_Ini. So I have no use for these dependencies. In my long-term attempt to get rid of ZF1 components, it sounds great to decouple them.

But again, I am fine with your decission. Shall I add the deps?

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.

2 participants