Skip to content

[Config] Adding the "info" to a missing option error messages #38176

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

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

weaverryan
Copy link
Member

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets None
License MIT
Doc PR Not needed

New error:

Screen Shot 2020-09-13 at 10 51 32 AM

I used the missing class just as an example. I hit this while building a new feature in Symfony. It occurred to me that if, for example, you activate some feature in config and that feature has a required sub-key, why not print the info() for that sub key to make it discoverable?

$ex = new InvalidConfigurationException(sprintf('The child node "%s" at path "%s" must be configured.', $name, $this->getPath()));
$message = sprintf('The child config "%s" under "%s" must be configured.', $name, $this->getPath());
if ($child->getInfo()) {
$message .= sprintf("\n\n Description: %s", $child->getInfo());
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'm not sure if the \n\n is a problem. It sure does help readability.

Copy link
Member

Choose a reason for hiding this comment

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

Having \n\n looks ok to me. Maybe we can remove the space before Description: , I would even remove the Description: prefix.

Copy link
Member

Choose a reason for hiding this comment

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

Expected value for the "%s" option: %s instead of Description?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think? Having a one liner as suggested by Nicolas seems good to me.

Copy link
Member

Choose a reason for hiding this comment

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

@weaverryan Not convinced?

@weaverryan
Copy link
Member Author

Fabbot failure appears false. Or at least, the referenced line was purposely changed to NOT include quotes 6 months ago: daf1c66#diff-7dae227729788bbef8c05ebb6f0f1345R210

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 14, 2020
@fabpot
Copy link
Member

fabpot commented Sep 19, 2020

Friendly ping

@fabpot
Copy link
Member

fabpot commented Sep 22, 2020

Thank you @weaverryan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants