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

Moving form to required #4753

Closed
wants to merge 1 commit into from
Closed

Moving form to required #4753

wants to merge 1 commit into from

Conversation

spiffyjr
Copy link
Contributor

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b66d821 on spiffyjr:patch-4 into 22554c4 on zendframework:develop.

@danizord
Copy link
Contributor

@spiffyjr Zend\Form should be optional since you can create the ServiceListener with a custom factory.

@spiffyjr
Copy link
Contributor Author

Yes, you can, but the default (i.e., the one 99.9% of people will use) includes the form. Technically it's not a dependency but having to write a new factory to avoid using it is wrong, imo.

We should include dependencies to make the component run by default or remove the extra dependencies that aren't required by default.

@weierophinney
Copy link
Member

We should include dependencies to make the component run by default or remove the extra dependencies that aren't required by default.

The latter (remove those that are not required by default) is where we've been heading.

MVC is a difficult one, as, technically, you can substitute your own service manager implementation if you want, making the requirements actually quite lean. I'm trying to figure out a way we can address this better, to be honest, as I want to make it clear what is needed at the most basic level, and what we require for the default implementation -- which are two separate things.

@weierophinney
Copy link
Member

@spiffyjr My point is that Form is not technically a required component. However, the default implementation of the MVC does require it. Let's address that dichotomy instead.

@spiffyjr
Copy link
Contributor Author

spiffyjr commented Jul 1, 2013

@weierophinney that's my recommended solution as well but needs to be addressed for more components than just MVC (although MVC is a big offender).

Thanks!

@spiffyjr spiffyjr closed this Jul 1, 2013
@weierophinney
Copy link
Member

@spiffyjr Actually, I merged a PR late last week that addressed a lot of this: #4584

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants