NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[POC] Introcude the request on CLI by default#21101

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

ro0NL wants to merge1 commit intosymfony:masterfromro0NL:framework/request

Closed

[POC] Introcude the request on CLI by default#21101

ro0NL wants to merge1 commit intosymfony:masterfromro0NL:framework/request

Conversation

Copy link

Contributor

ro0NL commentedDec 29, 2016•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes/no
Fixed tickets	#19396,#16968,#21027,#21043
License	MIT
Doc PR	symfony/symfony-docs#...

TLDR; it works.

This introduces a concept of having a request object on CLI. And it truly seems to be convenient.

The idea is to push a static request (Request::create) to the request stack, this fixes an issue in a way we have a (imo.) good defaulting strategy (things just work). No need for introducing context objects, additional class properties storing defaults, etc.

To be clear, this is not about actually callingKernel::handle($request) on CLI. However, in a way, we do have to consider theREQUEST event on this.

I did a quick setup for config proposal.

framework:request:host:foo.comscheme:httpsbase_url:'/sub/app_dev.php'# devbase_path:'/sub'# prod + dev

Additionally, when dealing with a "real" request we could opt-in to validate it against our definition, and perhaps throw aSuspiciousOperationException if things dont match.

This can also be a user-land strategy, so im curious about SF adopting it.

POC

003d711

carsonbot added Status: Needs Review Feature labels

Dec 29, 2016

Copy link

Member

wouterj commentedDec 29, 2016

Commands talk with the terminal, not HTTP land. I cannot see why having a HTTP land object in here is usefull. And I really can't see the usefullness if it's all just based on defaults configured in the configuration. These can be turned into parameters, which the command can use instead.

Copy link

ContributorAuthor

ro0NL commentedDec 30, 2016

Agree. It'sjust a defaulting stategy for that matter.

Technically we dont talk HTTP, we dont callhandle, converting the request into a response. We really dont request anything.

But i understand you concern, as it looks like we're crossing semantics, but it's really just a service.

Anyway, the asset+framework need a integration regarding this (imo). Not sure it should be an unsupported feature, but it could. And im not sure, how common using sub paths is anyway, i dont do it.

Copy link

Contributor

unkind commentedDec 30, 2016

@ro0NL default values is not always advantage, this kind of design may lead to unwanted behaviour. Often,fail-fast strategy is way more practical: let CLI-services withrequest_stack fail, make behavior of themexplicit. You would be surprised if you realized that some CLI listeners affect your web statistics.

Copy link

ContributorAuthor

ro0NL commentedDec 30, 2016•
edited
Loading

Agree, semantics get a big vague'ish here.

this kind of design may lead to unwanted behaviour.

Undertstand, it should be properly adapted. If adapted at all. It's really just a proposal how things can be done for that matter.

You would be surprised if you realized that some CLI listeners affect your web statistics.

Again, im not overseeing all the side effects here... but we dont talk HTTP. So im not sure what im overlooking.

And to go fully crazy; this would allow for your own appliction curl command (doing response introspection from CLI). I would never leave CLI i guess :)

We make everything useful, even without a webserver / webbrowser.

This allows integration withLynx for example.

Copy link

Contributor

unkind commentedDec 30, 2016•
edited
Loading

And to go fully crazy; this would allow for your own appliction curl command (doing response introspection from CLI). I would never leave CLI i guess
This allows integration with Lynx for example.

How it is related to your PR? What stops you implementing all this stuff without having default in your config?

Copy link

ContributorAuthor

ro0NL commentedDec 30, 2016

Nothing :) actually got some ideas. Nevertheless i find it a clean solution for the framework, in terms of standardizing things. This is truly about looking for a way tofix#19396 😞

But given the thumbs lets favor#21027