Skip to content

[HttpKernel] Do not enable HtmlDumper if the request is issued by curl #13915

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

Closed
wants to merge 2 commits into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Mar 12, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

If the request is issue by curl the output of dump function is in HTML, and so it's "dirty".

So, with this patch and the following code, I could get:

        die(dump([
            'key' => 'value',
            'date' => new \DateTime(),
        ]));

In web:

screenshot10

in cli:

screenshot12

\o/

@tucksaun
Copy link
Contributor

👍

@kingcrunch
Copy link
Contributor

What happens with wget -O-? Or other http-clients, that displays the raw response?

Because dump() is a debug function anyway, why not just remove, or comment it before using curl?

@lyrixx
Copy link
Member Author

lyrixx commented Mar 12, 2015

I do want to use curl and dump...

@csarrazi
Copy link
Contributor

@kingcrunch Taking a look at the code, it has no incidence on other HTTP clients, as long as they don't use the same user-agent as cURL.

@lyrixx A better approach may be to use the Cli dumper when a specific header is set (like X-Dumper-Format=Cli or X-Dumper-Format=Raw) from the HTTP client, which would switch the dumper from the HTML dumper to the console dumper (or even a raw dumper). This would be much more useful than simply using the user-agent to switch the dumper's strategy :)

@ghost
Copy link

ghost commented Mar 13, 2015

please, no user agent specific behaviour.

@kingcrunch
Copy link
Contributor

@csarrazi

Taking a look at the code, it has no incidence on other HTTP clients, as long as they don't use
the same user-agent as cURL.

Actually exactly that's the point. Now this introduces a special case for cURL and tomorrow I'll open a PR, that does the same for wget, because the output is "ugly" there too. The day after somebody else likes to get prettier output for their tool of choice and so on.

Another point against that: When I make a request against an URL I expect that same output a browser would see, because why else should I request an URL? Additionally there may be some ""tidy"-like functionality, or something like that, that this may break, because (I don't know exactly) it could end in invalid HTML.

Just my 2 cents :)

@ghost
Copy link

ghost commented Mar 13, 2015

you could do with with an accept header instead, if you're really trying to get text.

@lyrixx
Copy link
Member Author

lyrixx commented Mar 13, 2015

I totally understand your point of view. And indeed using accept or custom header is cleaner. But here, I really prefer simplicity .I don't want to add custom header each time I had to debug an API.

About wget and other: How many time did you use wget to debug an API ? How to you create a POST request with wget ? I might be possible, but seriously, no one debug with wget...

@ghost
Copy link

ghost commented Mar 13, 2015

but i do use other http clients that aren't curl or wget.

@ghost
Copy link

ghost commented Mar 13, 2015

specifically i use https://github.com/jakubroztocil/httpie which has an httpie/$version user agent. It's much better than curl for most cases imo.

{
// if the request is issued by curl, we do not enable HTML dumper;
if (false !== strpos($event->getRequest()->headers->get('user-agent'), 'curl/')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we change this condition to enable this behavior only for requests not initiated via CLI?

if ('cli' !== php_sapi_name()) {
    CliDumper::$defaultColors = true;
    CliDumper::$defaultOutput = 'php://output';

    return;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your comment. It should be 'cli' === ? Anyway, it will not work, because here the sapi IS fpm (or mod php) even if the request comes for curl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! Forget my comment. I just wondering if there is a way to detect requests made via the CLI, not via a particular user agent.

@nicolas-grekas
Copy link
Member

So, the use case is debugging with dump(), which means you are able (and in the process of) tweaking your code.
Instead of changing the behavior based on User-Agent (which looks not-ok for some), would it be acceptable to you @lyrixx (thinking about DX), to just add a config option to the debug bundle so you can choose the dumper (and maybe more options) there?

@lyrixx
Copy link
Member Author

lyrixx commented Mar 13, 2015

@jrobeson I use httpie too ; I could add an exception for HTTPie/.

But IMHO http works better for an API than for plain HTML. (anyway, that's out of PR scope ;) )

Do you really want to add another header every time you want to debug?

Are you feeling comfortable with the current workflow?

I have the feeling If we use this header, nobody will be aware of this feature, and people will continue to rant about the ugly output. And even If they know about the special header, they will not using it, because it's not straightforward (me first)

to address the issue Another point against that: When I make a request against an URL I expect that same output a browser would see, in this case, we could add a header to force an ugly output because the end-user decided to. Just for a reference, I made a screenshot of the same request before the PR:

screenshot13

=> So, again, Are you sure you want that by default?

@lyrixx
Copy link
Member Author

lyrixx commented Mar 13, 2015

Instead of changing the behavior based on User-Agent (which looks not-ok for some), would it be acceptable to you @lyrixx (thinking about DX), to just add a config option to the debug bundle so you can chose the dumper (and maybe more options) there?

@nicolas-grekas So each time I debug my app with curl, I need to edit my config.yml (or config_dev, whatever) file? It's not really straightforward. It's do-able, but not very easy ;)

@lyrixx
Copy link
Member Author

lyrixx commented Mar 13, 2015

Or, we could move the condition to the config layer, and use the expression language to evaluate it.
Like that, it solves all problems:

  • it's not related to one user agent;
  • it's disable-able;
  • it's possible to use with a custom header;
  • does not need to update the config file every time, the custom config could be committed.

@kingcrunch
Copy link
Contributor

@lyrixx

to address the issue Another point against that: "When I make a request against an URL I expect
that same output a browser would see, in this case, we could add a header to force an ugly output
because the end-user decided to.

So you recommend to enforce a header to get the normal result, but by default change the result depending on who is making the request? That sounds unintuitive.
At all I think when one use a HTMLDumper, but in some cases it doesn't dump html it is unintuitive. Is theres actually a reason you don't use good old var_dump() for your purpose?

@csarrazi
Copy link
Contributor

@lyrixx @nicolas-grekas What about injecting a custom header, as mentioned earlier?

A better approach may be to use the Cli dumper when a specific header is set (like X-Dumper-Format=cli or X-Dumper-Format=html) from the HTTP client, which would switch the dumper from the HTML dumper to the console dumper (or even a raw dumper if we wanted to have some raw output).

@lyrixx
Copy link
Member Author

lyrixx commented Mar 16, 2015

@kingcrunch

So you recommend to enforce a header to get the normal result, but by default change the result depending on who is making the request? That sounds unintuitive.

Yes, because here, we are talking about debug. A call to the dump function should not be committed, and is available only in dev environment. So I believe it's better to be pragmatic here.

At all I think when one use a HTMLDumper, but in some cases it doesn't dump html it is unintuitive.

I did not change at all the behavior of HTMLDumper.

Is theres actually a reason you don't use good old var_dump() for your purpose?

Yes, var_dump output is not as good as the dump function. that's why the var dumper component has been created.
And anyway, var_dump (with xdebug) output is still HTML, so with curl it's not useful.

@csarrazi I already answered your question

Do you really want to add another header every time you want to debug?

@ghost
Copy link

ghost commented Mar 16, 2015

If you folks do decide on something, then maybe also disable html_errors php.ini setting, so var_dump prints non html output too.

@csarrazi
Copy link
Contributor

Once again, the problem with the user-agent is that you may have unwanted side-effects.
Take for example guzzlehttp/guzzle's user-agent: User-Agent: Guzzle/4.0 curl/7.21.4 PHP/5.5.7. In this case, you will always receive cli markup for dump() regardless of the content, even though you would want it.

Also, another issue is that your markup won't display properly if you are using curl on windows, as the terminal doesn't handle ANSI output.

@lyrixx
Copy link
Member Author

lyrixx commented Mar 16, 2015

@jrobeson The issue with php.ini, is that is could be dependent of the current client.

@csarrazi The current implementation could be updated to avoid issue with guzzle.

@ghost
Copy link

ghost commented Mar 16, 2015

@lyrixx : the point is.. whatever we wend up switching to decide whether to show a "text mode" variable dump, then we should also twiddle the ini_setting, so var_dump() works too.

Let's not add a hack for guzzle too.

@lyrixx
Copy link
Member Author

lyrixx commented Mar 16, 2015

It will not be a hack, just a stricter comparison

@javiereguiluz
Copy link
Member

A lot of you are raising valid concerns about this, but at the end, I think Gregoire's original idea is the right thing to do.

Just think about this: right now, when you use dump() inside curl, you get this useless bunch of HTML code:

dump-html-curl

Now asks yourselves: is there any reason why you ever would want that HTML nonsense instead of this useful text dump?

dump-text-curl

@ghost
Copy link

ghost commented Mar 17, 2015

@javiereguiluz : nobody disagrees with the WHY, only the HOW.

@javiereguiluz
Copy link
Member

Some people said that the proposed behavior shouldn't be the default behavior. My comment was about: when/why would you want this proposal not to be the default behavior (when using curl)?

@nicolas-grekas
Copy link
Member

@javiereguiluz when you pipe the cURL output to a file that you then view with your browser :)

@javiereguiluz
Copy link
Member

@nicolas-grekas is that workflow common? I thought that in those cases people used browser extensions such as Postman and Advanced REST Client.

@LewisW
Copy link

LewisW commented Mar 17, 2015

What's wrong with something like?

alias symfonycurl="curl -H 'Content-Type: text/plain'"

No ugly agent sniffing hacks needed.

@lyrixx
Copy link
Member Author

lyrixx commented Mar 17, 2015

@LewisW:

1/ You need to know more things
2/ Autocomplete does not work anymore.

@Pierstoval
Copy link
Contributor

I personally use print_r or var_dump when debugging anything with Curl, and use json_encode when debugging a webservice with Postman or similar, so I wouldn't even use this feature 😕

@cs278
Copy link
Contributor

cs278 commented Mar 17, 2015

What's wrong with something like?

alias symfonycurl="curl -H 'Content-Type: text/plain'"
No ugly agent sniffing hacks needed.

Content-Type is the type of the request body, you mean Accept.

But that falls down if you are testing something that properly does content negotiation, say your bug is only in your JSON output which requires an Accept: application/json header you cannot easily debug it.

Using expression language seems like the best bet, users have the flexibility to tweak it match their requirements.

@mickaelandrieu
Copy link
Contributor

I'm totaly for this feature (I often have the same needs as you @lyrixx ), but I'm for @csarrazi solution: custom header instead of this kind of "hack" (not the good word, but I'm sure you understand).

We already use a custom X-Debug-Url, why not only a X-Dumper-Format ?

I like also the @nicolas-grekas suggest, this why environnements are made: have a specific configuration :)

@lyrixx
Copy link
Member Author

lyrixx commented Mar 17, 2015

Just pushed a new commit to be able to configure the expression...

@stof
Copy link
Member

stof commented Mar 17, 2015

@mickaelandrieu X-Debug-Url is a response header, not a request header. So it is something being read by the user, not something written by the user (which implies it knows about the fact that Symfony relies on this header to change behavior)

@mickaelandrieu
Copy link
Contributor

@stof true.

@lyrixx 👍 !

edit: wait, afaik httpKernel component doesn't rely on expression-language component. would you mind to add it as a "weak dependency" please ?

@csarrazi
Copy link
Contributor

@lyrixx I'm fine too with the EL! :)

@lyrixx
Copy link
Member Author

lyrixx commented Mar 17, 2015

@mickaelandrieu What are you talking?


namespace Symfony\Bundle\DebugBundle;

use Symfony\Component\Security\Core\Authorization\ExpressionLanguage as BaseExpressionLanguage;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason for not using Symfony\Component\ExpressionLanguage ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no good reason

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch ;)

@lyrixx
Copy link
Member Author

lyrixx commented Apr 2, 2015

It looks like nobody agree on this one. Closing it. I hope someone will find a better solution.

@lyrixx lyrixx closed this Apr 2, 2015
@mickaelandrieu
Copy link
Contributor

@lyrixx Too bad, I agreed with the selected option and I don't have seen anyone realy opposed to it /c @stof @aitboudad . Did you agree to re open it ?

@csarrazi
Copy link
Contributor

csarrazi commented Apr 2, 2015

@lyrixx I was fine too with the EL version, provided you switch to a function provider :)

@aitboudad
Copy link
Contributor

It's better than nothing 👍

@javiereguiluz
Copy link
Member

What about defining a second optional argument to the dump() function that defines the format to use:

dump($variable);          // outputs 'html'
dump($variable, 'html');  // outputs 'html'
dump($variable, 'txt');   // outputs 'txt'

Another idea, what about defining a new function with dump_*() variable name?

dump($variable);       // outputs 'html'
dump_html($variable);  // outputs 'html'
dump_txt($variable);   // outputs 'txt'

@Pierstoval
Copy link
Contributor

@javiereguiluz The second option sounds better, because the dump function originally accepts an variable number of arguments, like var_dump.

@javiereguiluz
Copy link
Member

Apparently, Laravel project is facing the same issues with the VarDumper component when debugging APIs. Here it is the PR proposed to fix it: laravel/framework#8556

@lukasgeiter
Copy link

@javiereguiluz The PR you linked doesn't fix anything with the component. It would just allows Laravel developers to easily replace the dumper instance with one of their own.

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.