Skip to content
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

Fix error on getNewInstance() when parent object property in uninitialised. #7705

Conversation

7ochem
Copy link
Contributor

@7ochem 7ochem commented Jan 13, 2022

Subject

When creating a new object instance, the admin will check if there is a parent object that needs to be appended. The appendParentObject() method will check the current value of the property that holds the parent object. But if this property is typed and not yet initialised, Symfony's PropertyAccess will throw an AccessException. This fix catches that exception and continues as if the current value is null.

Note:
I'm catching \Symfony\Component\PropertyAccess\Exception\AccessException. The \Symfony\Component\PropertyAccess\Exception\UninitializedPropertyException (child of AccessException) was introduced in Symfony 5.1 and thus not yet available in Symfony 4.4.

I am targeting the 3.x branch, because this is a bug.

Closes #7703.

Changelog

### Fixed
- Catch AccessException in AbstractAdmin::appendParentObject() to prevent an error when the property for the parent object is uninitialised.

@VincentLanglet
Copy link
Member

Note: I'm catching \Symfony\Component\PropertyAccess\Exception\AccessException. The \Symfony\Component\PropertyAccess\Exception\UninitializedPropertyException (child of AccessException) was introduced in Symfony 5.1 and thus not yet available in Symfony 4.4.

Can you add a TODO comment to change to UninitializedPropertyException when dropping support for Symfony < 5.1 ?

@7ochem 7ochem force-pushed the fix-error-when-parent-object-property-is-uninitialised branch from bce4d4d to e0d7982 Compare January 13, 2022 15:00
VincentLanglet
VincentLanglet previously approved these changes Jan 13, 2022
$value = $propertyAccessor->getValue($object, $propertyPath);
try {
$value = $propertyAccessor->getValue($object, $propertyPath);
} catch (AccessException $e) {
Copy link
Member

Choose a reason for hiding this comment

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

what happens if the entity does not have the getParent method? Would this try catch hide some potential problems?

Copy link
Member

Choose a reason for hiding this comment

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

I would say yes because "noSuchPropertyException" extends AccessException...

Dunno if we can filter the right case we want until support for Sf4.4 is dropped.

Should we check the message of the exception until the use of the new exception ?

Copy link
Contributor Author

@7ochem 7ochem Jan 14, 2022

Choose a reason for hiding this comment

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

PropertyAccessor throws this exception in 3 cases:

  1. The getter is used but a native PHP \Error was catched by PropertyAccessor, the PHP version is >= 7.4 and the message of the error is "Typed property .... must not be accessed before initialization"
  2. The property is public and PropertyAccessor detects that is was not set (not initialised)
  3. The getter is used but a native PHP \TypeError was catched by PropertyAccessor and the message of the error is "Return value ... must be of type ..., null returned" (in which case the property is untyped and uninitialised and the getter return type is not nullable)

what happens if the entity does not have the getParent method? Would this try catch hide some potential problems?

In case the property exists and is private/protected but the getParent method does not exist (or is private/protected) a \Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException is thrown.

Copy link
Member

Choose a reason for hiding this comment

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

NoSuchPropertyException

NoSuchPropertyException extends AccessException so it's caught too by your try/catch.
Same for NoSuchIndexException.

Maybe we should add a check if ($e instanceof NoSuchPropertyException || $e instanceof NoSuchIndexException) throw $e.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say yes because "noSuchPropertyException" extends AccessException...

Dunno if we can filter the right case we want until support for Sf4.4 is dropped.

Should we check the message of the exception until the use of the new exception ?

Ok... sharp, I've overlooked that "detail" 😄

I could add:

if (!$e instanceof UninitializedPropertyException && get_class($e) !== AccessException::class) {
    throw $e; // Re-throw. We only want to "ignore" AccessException (Sf < 5.1) and UninitializedPropertyException (Sf >= 5.1)
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it would be better with this re-throw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done. I also updated the todo message for this slightly.

Copy link
Member

Choose a reason for hiding this comment

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

Linter are failing

Copy link
Contributor Author

@7ochem 7ochem Jan 14, 2022

Choose a reason for hiding this comment

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

I fixed this one thing in my code, but now PHP CS Fixer is failing on other things. It is using PHP CS Fixer 3.5.0 which has been released 9 hours ago, so maybe this is a bug in that new release

I see PHP-CS-Fixer/PHP-CS-Fixer#6238. That's probably the one causing this.

@7ochem 7ochem force-pushed the fix-error-when-parent-object-property-is-uninitialised branch 2 times, most recently from b509316 to 0f5dccd Compare January 14, 2022 09:29
@VincentLanglet
Copy link
Member

@7ochem Can you rebase ? The linter build should be fixed.

…lised.

When creating a new object instance, the admin will check if there is a
parent object that needs to be appended. The appendParentObject() method
will check the current value of the property that holds the parent
object. But if this property is typed and not yet initialised, Symfony's
PropertyAccess will throw an AccessException. This fix catches that
exception and continues as if the current value is `null`.
@7ochem 7ochem force-pushed the fix-error-when-parent-object-property-is-uninitialised branch from 0f5dccd to b3a241d Compare January 17, 2022 08:40
@VincentLanglet VincentLanglet requested review from jordisala1991 and a team January 17, 2022 08:42
@VincentLanglet VincentLanglet merged commit 2a0b1ca into sonata-project:3.x Jan 18, 2022
@7ochem 7ochem deleted the fix-error-when-parent-object-property-is-uninitialised branch January 19, 2022 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting the parent object when creating a new instance gives error with uninitialised property (PHP >=7.4)
3 participants