Skip to content

Symfony Console Exception reporting doesn't parse style tags. #22552

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
rquadling opened this issue Apr 27, 2017 · 8 comments
Closed

Symfony Console Exception reporting doesn't parse style tags. #22552

rquadling opened this issue Apr 27, 2017 · 8 comments
Labels

Comments

@rquadling
Copy link
Contributor

rquadling commented Apr 27, 2017

Q A
Bug report? yes
Feature request? yes
Symfony version 2.8.19 though probably earlier

When an exception is generated by Symfony Console, the output is in a nice big red box.

throw new RuntimeException(
    sprintf(
        'Unable to locate the src directory within <fg=yellow;bg=red>%s</fg=yellow;bg=red>.'.PHP_EOL.
        'Try running with <fg=yellow;bg=red>--ignore-checkers</fg=yellow;bg=red>.',
        $input->getArgument('phpmaker-installation')
    )
);

image

But, unfortunately, if you put in style tags, they are rendered as literals.

If you override \Symfony\Component\Console\Output\OutputInterface::write() and unescape the tags, this will provide the output colouring, but not the correct lengths.

class MyOutput extends \Symfony\Component\Console\Output\Output
{
    /**
     * @inheritdoc
     *
     * Also removes the escaping of <fg> and <bg> tags.
     */
    public function write($messages, $newline = false, $options = self::OUTPUT_NORMAL)
    {
        parent::write(
            array_map(
                function ($message) {
                    return preg_replace('`\\\(?=</?[fb]g=)`', '', $message);
                },
                (array) $messages
            ),
            $newline,
            $options
        );
    }
}

image

The width of the block is determined by \Symfony\Component\Console\Application::stringWidth().

If this method was to remove the style tags before determining the length, then the length would/should be correct.

I'm aware that not all uses of '<xxxx>' will be style tags. Some may be literal and therefore escaping of the '<' (which is performed by \Symfony\Component\Console\Application::renderException() when it calls \Symfony\Component\Console\Formatter\OutputFormatter::escape()).

One idea on how to solve this would be to have an internal OutputFormatter that essentially removes known style tags. This formatter could wrap the string being passed to \Symfony\Component\Console\Application::stringWidth(), thus providing the correct string length.

The example code above doesn't take into account the other formats of the style tags (named styles).

Also, the \Symfony\Component\Console\Formatter\OutputFormatter::escape() method would need to ignore recognised style tags.

Happy to work on this, but any pointers, things I've missed, etc would be appreciated.

@ro0NL
Copy link
Contributor

ro0NL commented Apr 28, 2017

Imo. an exception message is a literal by definition. Hence this is changed to display the raw exception message; see #22021, #22142, #22188

Also note we cant really "cut" formatted messages, see #22225

Note this change was recently introduced; so im not sure we should revise it again. Imo the current behavior is fine. But we could always introduce an exception wrapper or so (FormattedException) which bypasses formatting. Not sure if worth it :)

@chalasr
Copy link
Member

chalasr commented Apr 28, 2017

But we could always introduce an exception wrapper

Exception messages should be raw text.. Adding style tags in console exceptions is like doing HTML in exception messages in an HTTP context. Nobody should rely on exceptions for tweaking the output IMO, just catch them and tweak the output accordingly instead, we have some nice APIs for that (*Style).

@rquadling
Copy link
Contributor Author

An exception message is a literal by definition

Exception messages should be raw text

I see no reason why an exception is must be plain text. I add semantic elements to the content of the message and the rendering deals with them appropriately.

I suppose, if I created a PR that satisfied my issues, correctly handled chopping of styled messages, had adequate tests and (if possible) was not BC breaking, how likely would this be accepted?

@ro0NL
Copy link
Contributor

ro0NL commented May 2, 2017

I see no reason why an exception is must be plain text.

Well to expose the message as-is. Exceptions come from everywhere, we cannot simply assume tags have a meaning, let alone they are correctly formatted. Opening al sorts of failure points, like proven before (where style tags broke, hence the change), while we're at a critical point in our app.

@rquadling
Copy link
Contributor Author

@ro0NL, I see what you are saying, but it is odd that the rendering of an exception both uses tags itself and will render tags if they aren't escaped.

It would seem that exceptions should just be plain text and no additional rendering at all.

I suppose the fact the PHP's exceptions have no context as such doesn't help. If there was a context, the rendering/view of the exception can appropriately handle the context content and act accordingly.

@ro0NL
Copy link
Contributor

ro0NL commented May 3, 2017

The message is wrapped in a style tag, so escaping it makes it a safe strategy.

Note the style system is regex based, so it comes with issues by design ;-) Allowing for nested styles, potentially incorrectly formatted, makes it very error prone and such.

Again, we could introduce some pointer interface to specify a safe message. But that doesnt solve any potential failures, which imo. we should avoid having at all.

@javiereguiluz
Copy link
Member

I think both sides have good points about this. I'd say ... if it's simple to fix this, let's fix it. If supporting this feature requires lots of changes in Symfony's code, let's not fix it.

@chalasr
Copy link
Member

chalasr commented Jul 22, 2017

We stopped trying to support parsing style tags in exception messages in #22142 as it was not possible to make it stable enough.
Closing here.

@chalasr chalasr closed this as completed Jul 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants