[OptionsResolver] Exception doesn't report the actual type used #11021

cleentfaar · 2014-05-30T09:42:30Z

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

Note to moderator: Stopwatch tests are failing randomly here; not related to this PR!
For more info, see #10454

UPDATE:

moved creation of printable value and type to separate methods for cleanliness
improved printable value by making a difference between an empty string and NULL.
restricted wrapping in quotes to only string values; further differentiating with other types.
when printable type is an object, it's classname get's returned instead of just 'object'

UPDATE 2:

applied feedback

UPDATE 3:

applied more feedback
squashed commits

sstok · 2014-05-31T07:23:20Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

                $option,
                $printableValue,
-                implode('", "', $allowedTypes)
+                implode('", "', $allowedTypes),
+                gettype($value)


This doesn't work nice with objects, its better to use something like is_object($value) ? get_class($value) : ettype($value) so you get the class-name instead of object ;)

Thanks, fixed

sstok · 2014-06-01T07:26:49Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

@@ -253,7 +259,7 @@ private function validateOptionsExistence(array $options)
            ksort($diff);

            throw new InvalidOptionsException(sprintf(
-                (count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.').' Known options are: "%s"',
+                (count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.') . ' Known options are: "%s"',


This should be reverted, Symfony CS doesn't use spaces around concatenating.

Thanks, reverted.

As a side note, I couldn't find a convention about concatenation in the CS nor in the conventions.

Could it get added to (one of) those pages?

fabpot · 2014-06-16T07:29:13Z

@cleentfaar Can you have a look at the comments and update this PR accordingly?

fabpot · 2014-06-16T07:30:00Z

@Tobion The only other option would be to abstract this into a component, but I find it quite overkill (I think @webmozart proposed to do that at some point as well.)

Tobion · 2014-06-16T07:36:57Z

@fabpot It could be worth considering we need this type of logic everywhere. And it's not implemented consistently yet.

cleentfaar · 2014-07-11T21:21:40Z

@fabpot Sorry, haven't had the time to get back on this PR lately.

Thanks for the feedback guys, I'm fixing it right now!

cleentfaar · 2014-07-11T21:54:30Z

~~Applied some of your feedback guys, could you respond to the replies I made to the remaining notes? I will then it get fixed properly and ready to merge.~~ All fixed!

@webmozart Did you manage to get a component ready for this like @fabpot mentioned?

If not I'm happy to get one set-up. Perhaps naming it something like Representation, Decoration, or Portrayal.

Not sure how I would make the createPrintableValue available though, seeing it's used in such exceptional cases, I suppose it would either need to be constructed in every piece of code it's used in, or making it static and thus callable in a single statement. Nice... but static stuff is something I always try to stay away from when I can. Let me know what you guys think of these suggestions.

Thanks!

cleentfaar · 2014-07-14T19:04:44Z

@fabpot @webmozart Any feedback on my last comment?

fabpot · 2014-07-15T08:13:27Z

If you are talking about creating a new component, that's not required for this PR and it should be discussed before writing code. So, this PR is fine by me as it is now.

stof · 2014-07-15T11:12:56Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

@@ -294,13 +300,14 @@ private function validateOptionsCompleteness(array $options)
    private function validateOptionValues(array $options)
    {
        foreach ($this->allowedValues as $option => $allowedValues) {
-            if (isset($options[$option])) {
+            if (array_key_exists($option, $options)) {


is it intended to change the way null values are handled ?

fabpot · 2014-07-23T15:16:47Z

@cleentfaar Any news?

cleentfaar · 2014-07-27T00:33:09Z

@fabpot Sorry been away from this branch for a while.

Thanks for the feedback guys, I've applied your comments and squashed everything down so it's ready for merging imo. Let me know if there's something left though.

P.S.
I still kind of liked the idea of having this functionality in my private projects, but imo it should not be coupled to the OptionsResolver like this. So I created a small library to do exactly what the createPrintableValue does, without the tight coupling and having it's own testcase.
I've called it the Humanizer and you can find it here: https://github.com/cleentfaar/humanizer. If interested, I'd be happy to make a PR for the places where Symfony can use it (the OptionsResolver component being the first I can think of).

Tobion · 2014-07-27T18:32:21Z

The components should not have such a dependency, which is why this code is duplicated everywhere. Also please rename the methods according to #10687: formatTypeOf and formatValue.
I guess you can just copy the methods with implementation from there.

cleentfaar · 2014-07-29T16:50:46Z

@Tobion I've updated the code again, replacing my own formatters with those of #10687 for sake of consistency.

I can't really agree though on the (potentially) prettifying of \DateTime instances for the OptionsResolver here; it cannot be controlled by the user and so we would end up simply having to force it to always be prettified, or never... I'm not sure that's something we should do?

fabpot · 2014-08-05T10:47:02Z

ping @webmozart

cleentfaar · 2014-08-05T11:18:38Z

BTW, the test failure is not related to this PR

stof · 2014-08-19T12:09:11Z

src/Symfony/Component/OptionsResolver/OptionsResolver.php

+     *
+     * @return string
+     */
+    protected function formatTypeOf($value)


all these helper methods should be private

fabpot · 2014-09-23T10:14:58Z

@cleentfaar Will you have time to update this PR according to @stof comments?

stof · 2014-09-25T18:09:43Z

this need to be rebased to fix conflicts with the huge refactoring done in #11716

cleentfaar · 2014-10-06T07:19:45Z

@stof Will fix this tonight, thanks!

…olver further improved message of the exception

cleentfaar · 2014-10-10T07:28:34Z

@stof Could you have a look again (rebased now)?

I've left out the parts where you could pass the format as an argument, as the user could not never control these arguments, the methods are only used during exception. Hope this is ok.

xabbuh · 2014-10-10T07:48:26Z

src/Symfony/Component/OptionsResolver/Options.php

@@ -228,7 +247,7 @@ public static function validateTypes(array $options, array $acceptedTypes)
                continue;
            }

-            $value = $options[$option];
+            $value       = $options[$option];


This should be reverted.

stof · 2014-10-10T08:02:19Z

src/Symfony/Component/OptionsResolver/Options.php

-                $printableValue,
-                implode('", "', $optionTypes)
+                static::formatValue($value),
+                static::formatTypesOf($optionTypes)


calling private methods must use self::, not static::, otherwise it is broken when extending the class, by trying to call it on the child class

cleentfaar · 2014-10-13T10:13:28Z

@stof @cordoval fixed

xabbuh · 2014-10-13T16:00:34Z

src/Symfony/Component/OptionsResolver/Options.php

@@ -79,13 +98,13 @@ class Options implements \ArrayAccess, \Iterator, \Countable
    public static function resolve(array $options, $defaults)
    {
        if (is_array($defaults)) {
-            static::validateNames($options, $defaults, true);
+            self::validateNames($options, $defaults, true);


validateNames() is public. Therefore, static must be kept here.

webmozart · 2014-10-16T09:31:20Z

This is replaced by #12156.

sstok reviewed May 31, 2014
View reviewed changes

cleentfaar changed the title ~~[OptionsResolver] Exception doesn't report actual type used~~ [OptionsResolver] Exception doesn't report the actual type used May 31, 2014

sstok reviewed Jun 1, 2014
View reviewed changes

cleentfaar added the OptionsResolver label Jun 16, 2014

stof reviewed Jul 15, 2014
View reviewed changes

stof reviewed Aug 19, 2014
View reviewed changes

cleentfaar force-pushed the issue-11020-exception-doesnt-report-actual-type-used branch from 2698bb3 to e4bff68 Compare October 6, 2014 07:03

cleentfaar force-pushed the issue-11020-exception-doesnt-report-actual-type-used branch from fba38bb to 98c4acc Compare October 10, 2014 06:25

clarified exception message to show the actual type passed to the res…

0de9a59

…olver further improved message of the exception

cleentfaar force-pushed the issue-11020-exception-doesnt-report-actual-type-used branch from 88d2d61 to 0de9a59 Compare October 10, 2014 07:05

simplified methods

004e832

xabbuh reviewed Oct 10, 2014
View reviewed changes

Cas Leentfaar added 2 commits October 10, 2014 09:54

removed bad indent

dd01472

removed bad indents

b7409f2

stof reviewed Oct 10, 2014
View reviewed changes

replaced 'static'-calls with 'self', removed alignment

18a8d10

xabbuh reviewed Oct 13, 2014
View reviewed changes

webmozart closed this Oct 16, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OptionsResolver] Exception doesn't report the actual type used #11021

[OptionsResolver] Exception doesn't report the actual type used #11021

cleentfaar commented May 30, 2014

sstok May 31, 2014

cleentfaar May 31, 2014

sstok Jun 1, 2014

cleentfaar Jun 1, 2014

fabpot commented Jun 16, 2014

fabpot commented Jun 16, 2014

Tobion commented Jun 16, 2014

cleentfaar commented Jul 11, 2014

cleentfaar commented Jul 11, 2014

cleentfaar commented Jul 14, 2014

fabpot commented Jul 15, 2014

stof Jul 15, 2014

fabpot commented Jul 23, 2014

cleentfaar commented Jul 27, 2014

Tobion commented Jul 27, 2014

cleentfaar commented Jul 29, 2014

fabpot commented Aug 5, 2014

cleentfaar commented Aug 5, 2014

stof Aug 19, 2014

fabpot commented Sep 23, 2014

stof commented Sep 25, 2014

cleentfaar commented Oct 6, 2014

cleentfaar commented Oct 10, 2014

xabbuh Oct 10, 2014

cleentfaar Oct 10, 2014

stof Oct 10, 2014

cleentfaar commented Oct 13, 2014

xabbuh Oct 13, 2014

webmozart commented Oct 16, 2014

[OptionsResolver] Exception doesn't report the actual type used #11021

[OptionsResolver] Exception doesn't report the actual type used #11021

Conversation

cleentfaar commented May 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 16, 2014

fabpot commented Jun 16, 2014

Tobion commented Jun 16, 2014

cleentfaar commented Jul 11, 2014

cleentfaar commented Jul 11, 2014

cleentfaar commented Jul 14, 2014

fabpot commented Jul 15, 2014

Choose a reason for hiding this comment

fabpot commented Jul 23, 2014

cleentfaar commented Jul 27, 2014

Tobion commented Jul 27, 2014

cleentfaar commented Jul 29, 2014

fabpot commented Aug 5, 2014

cleentfaar commented Aug 5, 2014

Choose a reason for hiding this comment

fabpot commented Sep 23, 2014

stof commented Sep 25, 2014

cleentfaar commented Oct 6, 2014

cleentfaar commented Oct 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cleentfaar commented Oct 13, 2014

Choose a reason for hiding this comment

webmozart commented Oct 16, 2014