-
-
Notifications
You must be signed in to change notification settings - Fork 882
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(jsonschema): make all required properties optional in PATCH operation with 'json' format #6478
base: 3.4
Are you sure you want to change the base?
Conversation
…ation with 'json' format by @ttskch see api-platform#6394
@@ -88,6 +88,19 @@ public function buildSchema(string $className, string $format = 'json', string $ | |||
return $schema; | |||
} | |||
|
|||
$isJsonMergePatch = 'json' === $format && 'PATCH' === $operation->getMethod() && Schema::TYPE_INPUT === $type; |
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.
$isJsonMergePatch = 'json' === $format && 'PATCH' === $operation->getMethod() && Schema::TYPE_INPUT === $type; | |
$isJsonMergePatch = 'json' === $format && 'PATCH' === $method && Schema::TYPE_INPUT === $type; |
} | ||
|
||
if (true === $skipRequiredProperties) { | ||
$definitionName .= self::PATCH_SCHEMA_POSTFIX; |
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.
Undefined constant
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.
Also, I think if we add this postfix here on Patch operations, it should also be added on other operations (Post, Put, Delete, Get, GetCollection, etc.)
if (null === ($skipRequiredProperties = $operation->getExtraProperties()['patch_skip_schema_required_properties'] ?? null)) { | ||
trigger_deprecation('api-platform/core', '3.4', "Set 'patch_skip_schema_required_properties' on extra properties as API Platform 4 will remove required properties from the JSON Schema on Patch request."); |
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.
if (null === ($skipRequiredProperties = $operation->getExtraProperties()['patch_skip_schema_required_properties'] ?? null)) { | |
trigger_deprecation('api-platform/core', '3.4', "Set 'patch_skip_schema_required_properties' on extra properties as API Platform 4 will remove required properties from the JSON Schema on Patch request."); | |
if (null === ($skipRequiredProperties = $operation->getExtraProperties()['skip_patch_json_schema_required_properties'] ?? null)) { | |
trigger_deprecation('api-platform/core', '3.4', "Set 'skip_patch_json_schema_required_properties' on extra properties as API Platform 4 will remove "required" JSON Schema node on Patch requests."); |
/** | ||
* @group legacy | ||
* TODO: find a way to keep required properties if needed | ||
*/ | ||
public function testPatchSchemaRequiredProperties(): void | ||
{ | ||
$this->tester->run(['command' => 'api:json-schema:generate', 'resource' => 'ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\PatchRequired\PatchMe', '--format' => 'json']); | ||
$result = $this->tester->getDisplay(); | ||
$json = json_decode($result, associative: true); | ||
|
||
$this->assertEquals(['b'], $json['definitions']['PatchMe']['required']); | ||
} |
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 also add a test when a user wants to force required
on a property on a Patch operation with "skip_patch_json_schema_required_properties" => true
.
See #6485 (comment) for this PR's resolution |
by @ttskch see #6394