Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Dec 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 calling Kernel::handle($request) on CLI. However, in a way, we do have to consider the REQUEST event on this.

I did a quick setup for config proposal.

framework:
  request:
    host: foo.com
    scheme: https
    base_url: '/sub/app_dev.php' # dev
    base_path: '/sub' # prod + dev

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

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

@wouterj
Copy link
Member

wouterj commented Dec 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.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 30, 2016

Agree. It's just a defaulting stategy for that matter.

Technically we dont talk HTTP, we dont call handle, 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.

@unkind
Copy link
Contributor

unkind commented Dec 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 with request_stack fail, make behavior of them explicit. You would be surprised if you realized that some CLI listeners affect your web statistics.

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 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 with Lynx for example.

@unkind
Copy link
Contributor

unkind commented Dec 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?

@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 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 to fix #19396 😞

But given the thumbs lets favor #21027

@ro0NL ro0NL closed this Dec 30, 2016
@ro0NL ro0NL deleted the framework/request branch December 30, 2016 11:20
@MrMitch
Copy link

MrMitch commented Jul 25, 2017

Thanks for the effort @ro0NL

@MrMitch
Copy link

MrMitch commented Jul 25, 2017

further discussion and investigation in #21027

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ro0NL @wouterj @unkind @MrMitch @carsonbot