[PropertyAccess] Show property path in all exception messages #13294

mpajunen · 2015-01-06T22:02:25Z

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

Add the property path to the exception message when trying to access a property of a scalar variable or null. For example:

$accessor->getValue((object) ['foo' => null], 'foo.bar'); // Throws UnexpectedTypeException
$old = 'Expected argument of type "object or array", "NULL" given'; // Old message
$new = 'PropertyAccessor requires a graph of objects or arrays to operate on, but it found type "NULL" while trying to traverse path "foo.bar" at property "bar".'; // New message

Checking the exception messages was added to the existing tests.

To make it possible to customize the exception messages, constructor was removed from UnexpectedTypeException. Instead the exception messages are built where the exception is thrown. (None of the other exception classes of the component had custom constructors.)

~~There are no BC breaks -- unless removing the constructor of an exception class is considered one.~~

The original PR included a potential BC break but the structure was changed somewhat.

The current version deprecates the old constructor for UnexpectedTypeException, but a BC layer is included.

fabpot · 2015-01-07T07:56:15Z

👍

Tobion · 2015-01-08T13:11:08Z

src/Symfony/Component/PropertyAccess/Exception/UnexpectedTypeException.php

@@ -18,8 +18,4 @@
 */
 class UnexpectedTypeException extends RuntimeException
 {
-    public function __construct($value, $expectedType)


this is a bc break

Yep, it is a theoretical BC break, hence my note at the bottom of the original PR comment. I don't see a straightforward way around it:

Using a new subclass of UnexpectedTypeException won't work because the constructor is already overloaded.

Using another exception class entirely would be a worse BC break.

Adding optional constructor arguments might be a smaller BC break, but those arguments would make no sense when the exception is used in PropertyPath.

I can of course make some changes, or add the BC break notes, or scrap the PR entirely.

I'd suggest to

change the exception in PropertyPath to Symfony\Component\PropertyAccess\Exception\InvalidArgumentException because that is more consistent with https://github.com/mpajunen/symfony/blob/property-access-exception-messages/src/Symfony/Component/PropertyAccess/PropertyAccessor.php#L57 and also makes more sense since it is a RuntimeException isntead of a LogicException (and should not be considered BC break since it's not intended to be catched).

adjust UnexpectedTypeException with optional extra params

That would work, I'll make the changes. Good point that the PropertyPath exception is not meant to be caught anyway.

Tobion · 2015-01-08T17:01:53Z

src/Symfony/Component/PropertyAccess/PropertyAccessor.php

@@ -97,7 +97,12 @@ public function setValue(&$objectOrArray, $propertyPath, $value)

            if ($overwrite) {
                if (!is_object($objectOrArray) && !is_array($objectOrArray)) {
-                    throw new UnexpectedTypeException($objectOrArray, 'object or array');
+                    throw new UnexpectedTypeException(sprintf(
+                        'Expected object or array, "%s" found when trying to write property "%s" of path "%s".',


Please change the message to PropertyAccessor requires a graph of objects or arrays to operate on, but it found type "%s" while trying to traverse path "%s" at property "%s".
Whether it's reading or writing is irrelavant at this point because it's always before doing so.

mpajunen · 2015-01-10T21:04:32Z

The messages have been changed. I also realized that using a new subclass of UnexpectedTypeException is indeed possible. The changes are now fully BC, but had to add a new exception class and skip the parent constructor when using it.

Could even revert the changes to PropertyPath now, but using a subclass of LogicException there still makes sense.

Tobion · 2015-01-11T01:02:31Z

src/Symfony/Component/PropertyAccess/Exception/UnexpectedPropertyTypeException.php

+    public function __construct($value, PropertyPathInterface $path, $index)
+    {
+        // Skip parent constructor.
+        RuntimeException::__construct(sprintf(


this works? looks rather hackish

Yeah, it looks a bit hackish indeed. Seems to work well enough though.

It isnt a hack. https://stackoverflow.com/questions/12589289/how-to-skip-execution-of-a-parent-method-to-execute-grandparent-method

call the parent constructor directly rather than using this hackish solution. It will be more robust when updating code. And then, trigger the error in the parent class only when using it directly:

if (__CLASS__ === get_class($this) { trigger_error(...); }

He cannot call the parent constructor because the arguments are different.

ah, I missed that

Tobion · 2015-01-11T01:19:14Z

If we go this route with a new subexception, then UnexpectedTypeException should get deprecated.

mpajunen · 2015-01-11T10:32:53Z

Alright, I can add the deprecation notes in that case.

I think this approach produces the cleanest end result. The parent constructor skipping can eventually be removed when the deprecated UnexpectedTypeException is removed.

To avoid similar problems in the future, it might be worth considering marking exception constructors internal, or using static factory methods, or avoiding custom constructors altogether.

mpajunen · 2015-01-13T20:19:57Z

Deprecation comment and error added. Change log updated as well. Should I update the UPGRADE files as well (or even add a file for 2.7)?

mpajunen · 2015-01-19T21:06:15Z

Adding a new exception class just to deprecate the old one didn't really seem that forward compatible. So I refactored the changes to use UnexpectedTypeException once again, but included the option to use the old arguments.

If the user code throws UnexpectedTypeException explicitly using the old constructor, there's a deprecation error for 2.7 and the BC layer can easily be removed for 3.0. If the constructor is not used, no user code changes will be required. A separate exception class would eventually force updating of the possible catch statements.

Use InvalidArgumentException instead of UnexpectedTypeException since this exception is not meant to be caught.

Show the property path in the exception message when trying to access a property of an invalid variable.

mpajunen · 2015-02-04T15:03:28Z

Is there something more I can do to get this merged?

(Rebased today.)

fabpot · 2015-02-04T15:10:15Z

looks good to me

fabpot · 2015-02-04T16:34:41Z

Thank you @mpajunen.

…essages (mpajunen) This PR was squashed before being merged into the 2.7 branch (closes #13294). Discussion ---------- [PropertyAccess] Show property path in all exception messages | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #12987 | License | MIT | Doc PR | N/A Add the property path to the exception message when trying to access a property of a scalar variable or null. For example: ```php $accessor->getValue((object) ['foo' => null], 'foo.bar'); // Throws UnexpectedTypeException $old = 'Expected argument of type "object or array", "NULL" given'; // Old message $new = 'PropertyAccessor requires a graph of objects or arrays to operate on, but it found type "NULL" while trying to traverse path "foo.bar" at property "bar".'; // New message ``` Checking the exception messages was added to the existing tests. ~~To make it possible to customize the exception messages, constructor was removed from `UnexpectedTypeException`. Instead the exception messages are built where the exception is thrown. (None of the other exception classes of the component had custom constructors.)~~ ~~There are no BC breaks -- unless removing the constructor of an exception class is considered one.~~ The original PR included a potential BC break but the structure was changed somewhat. The current version deprecates the old constructor for `UnexpectedTypeException`, but a BC layer is included. Commits ------- b286863 [PropertyAccess] Show property path in all exception messages

fabpot force-pushed the 2.7 branch from 77b6cca to dcfc338 Compare January 8, 2015 13:06

Tobion reviewed Jan 8, 2015
View reviewed changes

mpajunen force-pushed the property-access-exception-messages branch from 74f746b to 7dd6de6 Compare January 8, 2015 15:18

Tobion reviewed Jan 8, 2015
View reviewed changes

mpajunen force-pushed the property-access-exception-messages branch 6 times, most recently from 1fc18b4 to cae6d4a Compare January 10, 2015 20:54

Tobion reviewed Jan 11, 2015
View reviewed changes

mpajunen force-pushed the property-access-exception-messages branch from cae6d4a to 7b5ecac Compare January 13, 2015 20:14

mpajunen force-pushed the property-access-exception-messages branch from 7b5ecac to 2ca64e7 Compare January 13, 2015 23:18

fabpot added the PropertyAccess label Jan 16, 2015

mpajunen force-pushed the property-access-exception-messages branch 2 times, most recently from 28f258e to de04059 Compare January 19, 2015 20:48

mpajunen added 4 commits February 4, 2015 16:05

[PropertyAccess] Change invalid path type exception

e1a6a7f

Use InvalidArgumentException instead of UnexpectedTypeException since this exception is not meant to be caught.

[PropertyAccess] Add property path to exception messages

50546a0

Show the property path in the exception message when trying to access a property of an invalid variable.

[PropertyAccess] Test UnexpectedTypeException messages

04ae406

[PropertyAccess] Document UnexpectedTypeException changes

1f2f272

mpajunen force-pushed the property-access-exception-messages branch from de04059 to 1f2f272 Compare February 4, 2015 14:14

fabpot closed this Feb 4, 2015

mpajunen deleted the property-access-exception-messages branch February 12, 2015 19:10

Uh oh!

[PropertyAccess] Show property path in all exception messages #13294

[PropertyAccess] Show property path in all exception messages #13294

Uh oh!

Conversation

mpajunen commented Jan 6, 2015

Uh oh!

fabpot commented Jan 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpajunen commented Jan 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Jan 11, 2015

Uh oh!

mpajunen commented Jan 11, 2015

Uh oh!

mpajunen commented Jan 13, 2015

Uh oh!

mpajunen commented Jan 19, 2015

Uh oh!

mpajunen commented Feb 4, 2015

Uh oh!

fabpot commented Feb 4, 2015

Uh oh!

fabpot commented Feb 4, 2015

Uh oh!

Uh oh!