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

[WebServerBundle] Decouple server commands from the container #21190

Merged
merged 1 commit into from Jan 8, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Jan 7, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #20429
License MIT
Doc PR maybe

This removes the need for injecting the container in the new server:* commands, registering them as services when used in the framework and thus making them even more discoverable and extensible.
It would then be easy to reconsider extracting them in a WebServer component instead of having a bundle only. IMHO it would make sense to use these commands outside of the framework.

If the idea can be considered I'll add some tests at least ensuring that these commands are bootstrap-able. This must be done before that they are covered by the BC promise (3.3).

@chalasr
Copy link
Member Author

chalasr commented Jan 7, 2017

Here is what is needed to replace the bundle by a component and wire it in the framework master...chalasr:webserver-component
I can submit a PR, just tell me.

@fabpot
Copy link
Member

fabpot commented Jan 7, 2017

I'm :-0: on this change, but if others thinks it helps, why not.

But I'm 👎 on making it a component and tying it to the framework bundle is exactly what I wanted to avoid in the first place.

@chalasr
Copy link
Member Author

chalasr commented Jan 7, 2017

I saw it like "these commands should live without the framework" while it was "the framework should live without these commands", got it.

I think the first sentence stays good as a "bonus" and this change helps at making it true, decoupling the commands from the framework and from the container (right now they can't be used without both).

@mickaelandrieu
Copy link
Contributor

mickaelandrieu commented Jan 7, 2017

Regarding my previous comments, I'm totaly in favor of this decoupling.

But I'm 👎 on making it a component and tying it to the framework bundle is exactly what I wanted to avoid in the first place.

And again: why not a component and his bundle ?

In that case, the commands are not available unless you have the symfony/process component installed.

Also, as the Bundle "require" process component, I don't see how we cant avoid the autoloading of theses commands right now :)

@fabpot
Copy link
Member

fabpot commented Jan 8, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 2e63025 into symfony:master Jan 8, 2017
fabpot added a commit that referenced this pull request Jan 8, 2017
…ntainer (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[WebServerBundle] Decouple server commands from the container

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20429
| License       | MIT
| Doc PR        | maybe

This removes the need for injecting the container in the new `server:*` commands, registering them as services when used in the framework and thus making them even more discoverable and extensible.
It would then be easy to reconsider extracting them in a `WebServer` component instead of having a bundle only. IMHO it would make sense to use these commands outside of the framework.

If the idea can be considered I'll add some tests at least ensuring that these commands are bootstrap-able. This must be done before that they are covered by the BC promise (3.3).

Commits
-------

2e63025 [WebServerBundle] Decouple server:* commands from the container
@chalasr chalasr deleted the webserver-container-less branch January 8, 2017 11:52
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants