[php-nextgen] Fix validity checks for nullable properties that are required#23419
[php-nextgen] Fix validity checks for nullable properties that are required#23419JulianVennen wants to merge 2 commits intoOpenAPITools:masterfrom
Conversation
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/php-nextgen/model_generic.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/php-nextgen/model_generic.mustache:277">
P1: Required+nullable validation now relies on nullable-null tracking, but `ArrayAccess` setters/unsetters bypass that tracker, causing inconsistent and incorrect validation results.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| {{#required}} | ||
| if ($this->container['{{name}}'] === null) { | ||
| $invalidProperties[] = "'{{name}}' can't be null"; | ||
| if ($this->container['{{name}}'] === null{{#isNullable}} && !$this->isNullableSetToNull('{{name}}'){{/isNullable}}) { |
There was a problem hiding this comment.
P1: Required+nullable validation now relies on nullable-null tracking, but ArrayAccess setters/unsetters bypass that tracker, causing inconsistent and incorrect validation results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/resources/php-nextgen/model_generic.mustache, line 277:
<comment>Required+nullable validation now relies on nullable-null tracking, but `ArrayAccess` setters/unsetters bypass that tracker, causing inconsistent and incorrect validation results.</comment>
<file context>
@@ -274,8 +274,8 @@ class {{classname}} {{#parentSchema}}extends {{{parent}}}{{/parentSchema}}{{^par
{{#required}}
- if ($this->container['{{name}}'] === null) {
- $invalidProperties[] = "'{{name}}' can't be null";
+ if ($this->container['{{name}}'] === null{{#isNullable}} && !$this->isNullableSetToNull('{{name}}'){{/isNullable}}) {
+ $invalidProperties[] = "'{{name}}' {{^isNullable}}can't be null"{{/isNullable}}{{#isNullable}}is required"{{/isNullable}};
}
</file context>
There was a problem hiding this comment.
@JulianVennen thanks for the PR
can you please review this feedback which seems to be valid?
There was a problem hiding this comment.
Using $model['key'] = null does indeed not update the openAPINullablesSetToNull. It also doesn't do any other verification on whether the value is even nullable or of the correct type. I can update it in there, but I' not sure whether I should throw an exception if the variable is not nullable, as that might be considered a breaking change.
Using the normal model setters ($model->setKey(null)) does update it and also validates the parameter type.
If you have properties that are required which are set to null, the listInvalidProperties method will return them as invalid, even if they are nullable and therefore valid.
This seemingly was fixed for nullable properties without validation in the main php generator, but when using the php-nextgen generator (or the php generator with properties that have validation), the problem persisted.
I also updated the message for nullable properties, as the previous one didn't really make sense.
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)Summary by cubic
Fixes validation in generated PHP models so required nullable properties set to null are not flagged as invalid. Also clarifies the error for missing required nullable fields.
isNullableSetToNull()for required nullable props; null set explicitly is valid.php-nextgenandphptemplates.Written for commit 5d1bc86. Summary will update on new commits.