Skip to content

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

Merged
merged 6 commits into from
Aug 31, 2018

Conversation

komik966
Copy link
Contributor

@komik966 komik966 commented Aug 25, 2018

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

Connected with: #1749, symfony/symfony#28263

TODO:

  • make documentation normalizers (+ GraphQL?) aware of VOs
  • add unit tests

@komik966 komik966 force-pushed the constructor-relations-writtable branch 2 times, most recently from 6a300b3 to 5275c5a Compare August 26, 2018 08:19
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
@komik966 komik966 force-pushed the constructor-relations-writtable branch from 5275c5a to e1733fd Compare August 26, 2018 15:52
@komik966 komik966 force-pushed the constructor-relations-writtable branch from 37ad293 to 4309ffe Compare August 26, 2018 19:13
@komik966
Copy link
Contributor Author

For now I've made Swagger\DocumentationNormalizer to not add readOnly label when property is initializable.

But I think that it can be more guessable, consider example:
Users can change theirs password but not email:

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:

POST - input / output
{
  "email": "string",
  "password": "string"
}

PUT - input
{
  "password": "string"
}

PUT - output
{
  "email": "string",
  "password": "string"
}

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 PUT methods, but are ignored during request - check it:

Scenario: Update Value object with writable and non writable property

@mysiar mysiar requested review from dunglas and soyuka August 27, 2018 11:07
Copy link
Member

@dunglas dunglas left a 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);
}

/**
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@soyuka soyuka changed the title [WIP] Constructor relations writtable Constructor relations writtable Aug 29, 2018
@@ -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.',
Copy link
Contributor

Choose a reason for hiding this comment

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

a -> an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Fixed

@komik966 komik966 force-pushed the constructor-relations-writtable branch from ef0a26d to 508e6ee Compare August 29, 2018 12:35
@mysiar mysiar merged commit 6c15002 into api-platform:master Aug 31, 2018
@maks-rafalko
Copy link
Contributor

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.',
Copy link
Contributor

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.

Copy link
Contributor Author

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.

epourail pushed a commit to epourail/fork-apiplatform-core that referenced this pull request Sep 25, 2018
* 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
soyuka pushed a commit to CvekCoding/core that referenced this pull request Nov 2, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants