-
-
Notifications
You must be signed in to change notification settings - Fork 921
Constructor relations writtable #2178
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
Constructor relations writtable #2178
Conversation
6a300b3
to
5275c5a
Compare
implement constructor params relation denormalization drop identifier getter typehint because of doctrine listeners problem tests for cyclic relationship drop constructor cycle deps (impossible to handle), add iri and nullable param test test put method remove return types from test entities to pass low deps test fix cs fix phpstan adjust to changes in symfony/serializer PR
5275c5a
to
e1733fd
Compare
37ad293
to
4309ffe
Compare
For now I've made But I think that it can be more guessable, consider example: class User
{
private $email;
private $password;
public function __construct(string $email) { $this->email = $email; }
public function getEmail(): string { return $this->email; }
public function getPassword(): string { return $this->password; }
public function setPassword(string $password): void { $this->password = $password; }
} Then documentation should looks like this:
Such documentation behavior will allow us generating CRUD clients with distinction between what should appear in create form and what in update form, without the need to define serialization groups. If we're ok with this, I prefer to create it in different PR. For now non writable properties are included in documentation for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a minor change, then 👍 on my side
@@ -127,6 +129,91 @@ public function denormalize($data, $class, $format = null, array $context = []) | |||
return parent::denormalize($data, $class, $format, $context); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mark it as @internal
because we'll remove it at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -299,6 +300,10 @@ public function testNormalize() | |||
'type' => 'string', | |||
'description' => 'This is a name.', | |||
]), | |||
'description' => new \ArrayObject([ | |||
'type' => 'string', | |||
'description' => 'This is a initializable but not writable property.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a -> an
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Fixed
ef0a26d
to
508e6ee
Compare
please, don't forget to update documentation. It's so painful when documentation is updated post-factum by someone who spends hours debugging, not in advance. thanks for this feature btw! |
} else { | ||
throw new MissingConstructorArgumentsException( | ||
sprintf( | ||
'Cannot create an instance of %s from serialized data because its constructor requires parameter "%s" to be present.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this error be more consumer friendly on serialization? it's exposing implementation detail to the consumer which they don't care about: the class name, overly verbose message etc... all the consumer needs to know is that they didn't provide the some parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code comes from symfony/serializer
(see method comment). So I'm not sure if we should change it. But I've similar opinion. Ideally would be to show such errors the same way as validation errors (as map: property => error message). I think that non nullable and without default value constructor parameters are equivalent to class attributes with NotBlank
constraint (with some exceptions) and errors should be presented the same way.
* test relations with writable constructor parameters implement constructor params relation denormalization drop identifier getter typehint because of doctrine listeners problem tests for cyclic relationship drop constructor cycle deps (impossible to handle), add iri and nullable param test test put method remove return types from test entities to pass low deps test fix cs fix phpstan adjust to changes in symfony/serializer PR * in swagger mark property as readOnly when is not writable and initializable * mark method internal * fix typo in test
* test relations with writable constructor parameters implement constructor params relation denormalization drop identifier getter typehint because of doctrine listeners problem tests for cyclic relationship drop constructor cycle deps (impossible to handle), add iri and nullable param test test put method remove return types from test entities to pass low deps test fix cs fix phpstan adjust to changes in symfony/serializer PR * in swagger mark property as readOnly when is not writable and initializable * mark method internal * fix typo in test
Connected with: #1749, symfony/symfony#28263
TODO: