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

Move type check when deserializing into the graph navigator #1080

Merged
merged 1 commit into from
May 3, 2019

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented May 3, 2019

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

@goetas goetas merged commit cb3e167 into master May 3, 2019
@@ -194,6 +194,10 @@ public function accept($data, ?array $type = null)
continue;
}

if (!$propertyMetadata->type) {
throw new RuntimeException(sprintf('You must define a type for %s::$%s.', $propertyMetadata->class, $propertyMetadata->name));
Copy link

@surikman surikman Jun 28, 2019

Choose a reason for hiding this comment

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

@goetas Could you describe please, why was this check necessary?

My use-case:
I have an AbstractCommand => class in library outside of project and then I have own Command (e.g.: CreateUser) which extends AbstractCommand and my command has some properties (e.g.: username, email...)
I need to deserialize request such as {"username": "JohnDoe", "email":"john.doe@example.com"} into my CreateUser command (BUT AbstractCommand has special property metadata.

I don't need to use this property in my project, but Library works with this property.
When I updated from 3.0.1 to 3.1.0 I got an error message:
You must define a type for MyExternalLibraryAKACommandBus\AbstractCommand::$metadata

Is there any chance to skip this throwing exception instead of doing changes in this library?

Choose a reason for hiding this comment

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

I opened an issue: #1102

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