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

Do not access not-existing properties #1141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thePanz
Copy link

@thePanz thePanz commented Nov 13, 2019

Q A
Bug fix? yes/no
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes/no
License MIT

This also checks that the accessor is not trying to access a property that does not exist on the object. See the getValue() with return $o->$name ?? null;

This is happening in the following case:

 class ClassA {
    /**
     * @var ClassB[]|null
     * @Type("array<App\Model\ClassB>")
     */
    public $sortimentierungen;
}
class ClassB {
    /**
     * @var string|null
     * @Type("string")
     */
    public $code;
}

And we deserialize from an XML like:

<ClassA>
  <ClassB />
  <ClassB><code>123</code></ClassB>
</ClassA>

The first element of type ClassB is defined, but with no properties: this leads the serializer trying to read from a not existing property stdClass->code. And getting the PHP notice Notice: Undefined property: stdClass::$code.

@thePanz thePanz changed the title Add PHP typehints to DefaultAccessor closures Do not access not-existing properties Nov 13, 2019
@goetas
Copy link
Collaborator

goetas commented Nov 14, 2019

Thanks for the PR!
Much appreciated!

This also checks that the accessor is not trying to access a property that does not exist on the object. See the getValue() with return $o->$name ?? null;

Can you create a test case for this?

@thePanz
Copy link
Author

thePanz commented Nov 14, 2019

I am trying to create a test case for this, mimicking the bug I had in my project.
Looks like it is more complicated than I expected, I'll post an update ASAP :)

@thePanz
Copy link
Author

thePanz commented Nov 19, 2019

@goetas looks like the whole issue was caused by a weird data being serialized.
The property @var Classb[] was an array, but of a different class.

I will try to alter this MR to provide a better exception reporting the class mismatch, so to help to identify the issue

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.

2 participants