[2.2] add support for optional custom error message for Configura... #5094

cordoval · 2012-07-28T18:39:56Z

RFC: ref #1666

Basically this is the start, but i wanted to ask which one of these options is better:

extend/override the base Exception class to include the custom error message
this requires a modification of the throw specific expection passing Exception($this) meaning it is getting
the current node or the node from which the exception is being thrown
propagate the changes in every class that throws exception similar than this PR
this would require changes in every particular class

Please vote whether 1 or 2 is more fit

Thanks to all symfony2 community :D

cordoval · 2012-10-02T22:32:39Z

@stof any updates on this one? or is left to starve to death?

stof · 2012-10-02T22:49:24Z

@cordoval I don't think adding yet another method is a good idea. A better solution would be to use the info attached to the node to enhance the error message

fabpot · 2012-10-03T06:30:53Z

+1 for @stof idea.

fabpot · 2012-10-05T17:05:21Z

@cordoval Can you work on an evolution of this PR based on the feedback?

cordoval · 2012-10-05T17:14:14Z

@stof @fabpot yes i will start tonight sorry for the delay 👍

fabpot · 2012-10-05T17:20:31Z

@cordoval Thanks a lot.

cordoval · 2012-10-07T06:50:04Z

I have thought in two ways to refactor:

$message = sprintf(
    'Invalid configuration for path "%s": %s',
    $this->getPath(),
    $invalid->getMessage()
);
if ($this->getInfo() != null) {
    $message .= sprintf("\nError message: %s.", $this->getInfo());
}
$ex = new InvalidConfigurationException($message, $invalid->getCode(), $invalid);
$ex->setPath(...)

throw $ex;

into

$message = sprintf(
    'Invalid configuration for path "%s": %s',
    $this->getPath(),
    $invalid->getMessage()
)

$ex = new InvalidConfigurationException($this->addErrorHintMessageIfPresent($message), $invalid->getCode(), $invalid);
$ex->setPath(...)

throw $ex;

and have the helper method to the BaseNode class to avoid locs everywhere

or even implementing this into the exception base class or some other but then do:

$ex->setPath(...);
$ex->setInfo($this->getInfo());

Any preference? @stof @fabpot

cordoval · 2012-10-07T06:56:45Z

I will also work on changes to the documentation after this is re-factored and put into shape for merge.

schmittjoh · 2012-10-07T07:29:15Z

I don't think this works that well. With such an implementation, all you could add is a description of the node in question.

The reason why error message does not work is simply that there might be many reasons for why something is going wrong. So, for each of these reasons, you would need to add a different message. If you really would like to do that, this could work by adding first an id for each message that could occur (or generate one), and then having a map on the node which replaces the generic message with the custom one. This would be a bit of work though.

If you instead only would like to add a description of the node, I'm not really convinced this is useful either as the error message still is the generic one, and not a custom error message.

cordoval · 2012-10-07T07:40:40Z

@schmittjoh the idea for the info in this case is to provide a sample peek of what the configuration looks like or a hint why usually things went wrong in terms of how the configuration is used. We are not trying to map a message to every problem as in the case where we have constraints and we want to specify things. I think for that we have the custom nodes that can be extended for a given specific data constraint/requirements, like the NumericNode or IntegerNode which were PRed. This is a small tiny helper hint to have. But yeah i agree with you that it is no more than that.

Maybe @lsmith77 wanted to have just that or maybe more? I think this is like the run-time version of config:dump-reference command.

lsmith77 · 2012-10-07T21:52:53Z

@schmittjoh if there are multiple causes you can list them all. i expect this to mostly be used to clarify things based on issue tickets. ie. users express some confusion about an error message, and then you add some clarifications. sort of an evolutionary process.

stof · 2012-10-13T18:00:51Z

src/Symfony/Component/Config/Definition/ArrayNode.php

@@ -205,6 +205,9 @@ protected function finalizeValue($value)
            if (!array_key_exists($name, $value)) {
                if ($child->isRequired()) {
                    $msg = sprintf('The child node "%s" at path "%s" must be configured.', $name, $this->getPath());
+                    if ($this->getInfo() != null) {


it should be null !== $this->getInfo()

stof · 2012-10-13T18:03:16Z

@cordoval Please update all nodes according to my comments and then squash your commits.

fabpot · 2012-12-06T11:37:21Z

What is the status of this PR?

cordoval · 2012-12-06T12:50:35Z

@fabpot i will review it tonight thanks

cordoval · 2012-12-07T12:55:53Z

gah, i got swamped at my work, anyone be willing to take a look and what is left please? if not then i can say i can try looking at beginning of next week

cordoval · 2012-12-08T23:47:41Z

@fabpot ok iam working on this now, i got time, i will basically finish the tests which i think is the only thing left

cordoval · 2012-12-09T00:05:22Z

👍 /cc @stof @lsmith77

fabpot · 2012-12-10T12:58:32Z

Looks good to me but we repeat the same code over and over again. Can we try to make the code more future proof. what about adding a setHint() method to the exception that does the work?

…info [symfony#1666] RFC: add support for optional custom error message for Configuration validation errors [symfony#1666] remove method that was previously added on NodeDefinition since we will reuse info method [symfony#1666] remove errorMessage methods introduced in BaseNode class since we are going through info this time [symfony#1666] reuse getInfo message for error output in exception for ScalarNode class [symfony#1666] clean up of ScalarNode adding of info exception message [symfony#1666] reuse getInfo message for error output in exception for VariableNode class [symfony#1666] correct a typo explicited -> made explicit [symfony#1666] reuse getInfo message for error output in exception for NumericNode class [symfony#1666] reuse getInfo message for error output in exception for FloatNode and IntegerNode [symfony#1666] reuse getInfo message for error output in exception for EnumNode class [symfony#1666] reuse getInfo message for error output in exception for BooleanNode class [symfony#1666] reuse getInfo message for error output in exception for BaseNode class [symfony#1666] reuse getInfo message for error output in exception for ArrayNode class [symfony#1666] remove test on TreeBuilder test getInfo since it becomes redundant as info already is tested [symfony#1666] fix test to use getInfo for ScalarNodeTest address comments swtich to if (->getInfo() != null) {

cordoval · 2012-12-14T11:18:40Z

@fabpot working on this right now

cordoval · 2012-12-14T12:13:45Z

@fabpot ok done, the error is because of another failing test in master, but so everything else is passing.

👍 thanks for helping me out, I think some more refactoring could be done but it could also be overkill

So all good now

fabpot · 2012-12-14T12:15:55Z

src/Symfony/Component/Config/Definition/ArrayNode.php

@@ -204,9 +204,12 @@ protected function finalizeValue($value)
        foreach ($this->children as $name => $child) {
            if (!array_key_exists($name, $value)) {
                if ($child->isRequired()) {
-                    $msg = sprintf('The child node "%s" at path "%s" must be configured.', $name, $this->getPath());
-                    $ex = new InvalidConfigurationException($msg);
+                    $ex = new InvalidConfigurationException(sprintf('The child node "%s" at path "%s" must be configured.',


Can you revert to everything on one line? (same below)

Can you do the same for the other exceptions? Thanks.

fabpot · 2013-01-09T08:41:36Z

@cordoval Any news on my comments?

cordoval · 2013-01-09T09:36:14Z

@fabpot can this still get on 2.2 release?

fabpot · 2013-01-09T09:47:48Z

Yes, but it must be mergeable before Friday.

fabpot · 2013-04-22T04:38:40Z

@cordoval Any chance you address my latest comments? I'm about to close it as this is not something we must have.

fabpot · 2013-08-09T06:33:57Z

Closing as this code have serious drawbacks. Feel free to reopen whenever the code is ready.

cordoval · 2013-08-09T07:35:15Z

ok @fabpot i will bookmark this to work on, sorry about the delay, 👶

stof reviewed Oct 13, 2012
View reviewed changes

cordoval added 3 commits December 14, 2012 06:36

[symfony#1666] add setHint method refactor on exception

2c57af6

[symfony#1666] comply with cs

bdebed5

cs missing spot

a15fbe6

fabpot reviewed Dec 14, 2012
View reviewed changes

revert to everything into one line

c5bfe76

fabpot closed this Aug 9, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.2] add support for optional custom error message for Configura... #5094

[2.2] add support for optional custom error message for Configura... #5094

cordoval commented Jul 28, 2012

cordoval commented Oct 2, 2012

stof commented Oct 2, 2012

fabpot commented Oct 3, 2012

fabpot commented Oct 5, 2012

cordoval commented Oct 5, 2012

fabpot commented Oct 5, 2012

cordoval commented Oct 7, 2012

cordoval commented Oct 7, 2012

schmittjoh commented Oct 7, 2012

cordoval commented Oct 7, 2012

lsmith77 commented Oct 7, 2012

stof Oct 13, 2012

stof commented Oct 13, 2012

fabpot commented Dec 6, 2012

cordoval commented Dec 6, 2012

cordoval commented Dec 7, 2012

cordoval commented Dec 8, 2012

cordoval commented Dec 9, 2012

fabpot commented Dec 10, 2012

cordoval commented Dec 14, 2012

cordoval commented Dec 14, 2012

fabpot Dec 14, 2012

cordoval Dec 14, 2012

fabpot Dec 14, 2012

fabpot commented Jan 9, 2013

cordoval commented Jan 9, 2013

fabpot commented Jan 9, 2013

fabpot commented Apr 22, 2013

fabpot commented Aug 9, 2013

cordoval commented Aug 9, 2013

[2.2] add support for optional custom error message for Configura... #5094

[2.2] add support for optional custom error message for Configura... #5094

Conversation

cordoval commented Jul 28, 2012

cordoval commented Oct 2, 2012

stof commented Oct 2, 2012

fabpot commented Oct 3, 2012

fabpot commented Oct 5, 2012

cordoval commented Oct 5, 2012

fabpot commented Oct 5, 2012

cordoval commented Oct 7, 2012

cordoval commented Oct 7, 2012

schmittjoh commented Oct 7, 2012

cordoval commented Oct 7, 2012

lsmith77 commented Oct 7, 2012

stof Oct 13, 2012

Choose a reason for hiding this comment

stof commented Oct 13, 2012

fabpot commented Dec 6, 2012

cordoval commented Dec 6, 2012

cordoval commented Dec 7, 2012

cordoval commented Dec 8, 2012

cordoval commented Dec 9, 2012

fabpot commented Dec 10, 2012

cordoval commented Dec 14, 2012

cordoval commented Dec 14, 2012

fabpot Dec 14, 2012

Choose a reason for hiding this comment

cordoval Dec 14, 2012

Choose a reason for hiding this comment

fabpot Dec 14, 2012

Choose a reason for hiding this comment

fabpot commented Jan 9, 2013

cordoval commented Jan 9, 2013

fabpot commented Jan 9, 2013

fabpot commented Apr 22, 2013

fabpot commented Aug 9, 2013

cordoval commented Aug 9, 2013