GenConfigCommand - print wsdl loading exceptions when output is verbose#610
Conversation
veewee
left a comment
veewee
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I added some small remarks.
| $wsdl = $this->loadWsdl($wsdlUri); | ||
| } catch (\Throwable $e) { | ||
| $io->warning('Could not load the provided WSDL with default engine options.'); | ||
| if ($output->isVerbose()) { |
There was a problem hiding this comment.
Maybe better to display this always? I don't think people will run this command with -v very often.
There was a problem hiding this comment.
I am happy to change this. Although I have a couple of arguments against showing this at all times
For me the story went:
- Something went wrong. I got a warning, fine. But I want to know more
- Ran --help -> Saw verbose output
- Re-ran verbose and expected extra info on what went wrong
- Some exceptions come from deep down, dependencies,... and can be quite cryptic. The already in place warning looks more user friendly (For example, bad xml formats would print something like Unexpected character
<;in a big red block - when unexpected this seems like bad UX to be). - I feel like using -v and -vv is somewhat in line with the --help message (From Symfony Console)
# /bin/soap-client wizard --help
Options:
-v|vv|vvv, --verbose Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debugTLDR: I think it's a bit aggressive UX to always display these. But I am happy to make this change.
There was a problem hiding this comment.
The problem is that: even though the config can be generated, you won't be able to use that generated config given that the loading of WSDL will fail in the next command you run (especially if you use the wizard).
If the issue is e.g. at connection level rather than on XML level, this will give you quite an idea already to start with.
Maybe we should display the message less aggressively since it's not blocking (at least for this command) - but always display some more details than be vague about what error happened exactly?
There was a problem hiding this comment.
Completely makes sense. I felt it would be too aggressive, but you made very good points
I have updated this in 1153321
Summary
- Use error output for the class/message
- Pass arrays to the Symfony IO so it can handle multi line formatting for us. Keeping larger exception messages and the stack trace nicely formatted
Note: Would you like for me to squash/rebase this branch? (get rid of the formatting commits etc?)
|
|
||
| // Create configuration objects | ||
| $typeDestination = new Destination($baseDir . DIRECTORY_SEPARATOR . 'Type', $namespace . '\\Type'); | ||
| $typeDestination = new Destination($baseDir.DIRECTORY_SEPARATOR.'Type', $namespace.'\\Type'); |
There was a problem hiding this comment.
Not sure why you needed to apply all these style changes, because this is not in line with the style in the other classes. (same for pretty much all style changes you did in this file)
What failed exactly?
There was a problem hiding this comment.
I am sorry about the formatting changes. I think a Laravel pint auto command slipped through.
I cleaned this up in 4287b26 and used the included phpcbf instead.
| $io->warning('Could not load the provided WSDL with default engine options.'); | ||
| if ($output->isVerbose()) { | ||
| $io->text('<fg=red>'.$e::class.'</>'); | ||
| $io->text($e->getMessage()); |
There was a problem hiding this comment.
maybe also better to put in red.
Isn't there a helper in $io to style the error message?
There was a problem hiding this comment.
Yes, It felt a bit aggressive since its a big red block, this is just some red text.
Though you make a good point, and it is probably more inline with the other output (Such as warning)
…se (#610) GenerateConfigCommand - always print wsdl loading errors
2 participants
Context
I spent some time debugging a local WSDL file recently. The path was incorrect but the exception was silently ignored, making it tedious to figure out what went wrong. The exception message pointed me straight to the issue.
I was using 5.x - I noticed the path issue I had was resolved in php-soap/wsdl#35. (thank you)
Changes
-vshows the exception class and message-vvalso shows the stack traceSorry for the formatting changes, without them I was unable to pass all pre-commit checks.