[php-nextgen] Discriminator class detection uses wrong namespace#23287
[php-nextgen] Discriminator class detection uses wrong namespace#23287
Conversation
There was a problem hiding this comment.
4 issues found across 11 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/ObjectSerializer.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/php-nextgen/ObjectSerializer.mustache:502">
P2: Resolve discriminator values through the generated mapping before building the subclass name. Custom discriminator mappings will otherwise deserialize to the base model instead of the mapped subtype.</violation>
</file>
<file name="samples/client/petstore/php-nextgen/OpenAPIClient-php/src/Model/DiscriminatorBase.php">
<violation number="1" location="samples/client/petstore/php-nextgen/OpenAPIClient-php/src/Model/DiscriminatorBase.php:249">
P1: Passing `null` as the fallback here clears the discriminator default, so a freshly constructed `DiscriminatorBase` is invalid and `getType(): string` can return `null`.</violation>
</file>
<file name="samples/client/petstore/php-nextgen/OpenAPIClient-php/src/Model/DiscriminatorChild.php">
<violation number="1" location="samples/client/petstore/php-nextgen/OpenAPIClient-php/src/Model/DiscriminatorChild.php:28">
P1: Add the missing imports for `InvalidArgumentException`, `ReturnTypeWillChange`, and `ObjectSerializer`; this subclass currently resolves them into the model namespace and will break on serialization, attributes, or the null-check exception path.</violation>
</file>
<file name="samples/client/petstore/php-nextgen/OpenAPIClient-php/docs/Model/DiscriminatorChild.md">
<violation number="1" location="samples/client/petstore/php-nextgen/OpenAPIClient-php/docs/Model/DiscriminatorChild.md:1">
P3: Use a single Markdown heading marker for the model title.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Initialize discriminator property with the model name. | ||
| $this->container['type'] = static::$openAPIModelName; | ||
|
|
||
| $this->setIfExists('type', $data ?? [], null); |
There was a problem hiding this comment.
P1: Passing null as the fallback here clears the discriminator default, so a freshly constructed DiscriminatorBase is invalid and getType(): string can return null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/php-nextgen/OpenAPIClient-php/src/Model/DiscriminatorBase.php, line 249:
<comment>Passing `null` as the fallback here clears the discriminator default, so a freshly constructed `DiscriminatorBase` is invalid and `getType(): string` can return `null`.</comment>
<file context>
@@ -0,0 +1,414 @@
+ // Initialize discriminator property with the model name.
+ $this->container['type'] = static::$openAPIModelName;
+
+ $this->setIfExists('type', $data ?? [], null);
+ }
+
</file context>
There was a problem hiding this comment.
Discriminator type should be logically always required, but nothing in OpenApi documentation states it explicitly.
Currently there is no hard check in generated code, it is up to developer to design the API correctly.
| * Do not edit the class manually. | ||
| */ | ||
|
|
||
| namespace OpenAPI\Client\Model; |
There was a problem hiding this comment.
P1: Add the missing imports for InvalidArgumentException, ReturnTypeWillChange, and ObjectSerializer; this subclass currently resolves them into the model namespace and will break on serialization, attributes, or the null-check exception path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/php-nextgen/OpenAPIClient-php/src/Model/DiscriminatorChild.php, line 28:
<comment>Add the missing imports for `InvalidArgumentException`, `ReturnTypeWillChange`, and `ObjectSerializer`; this subclass currently resolves them into the model namespace and will break on serialization, attributes, or the null-check exception path.</comment>
<file context>
@@ -0,0 +1,397 @@
+ * Do not edit the class manually.
+ */
+
+namespace OpenAPI\Client\Model;
+
+/**
</file context>
| namespace OpenAPI\Client\Model; | |
| namespace OpenAPI\Client\Model; | |
| use InvalidArgumentException; | |
| use ReturnTypeWillChange; | |
| use OpenAPI\Client\ObjectSerializer; |
There was a problem hiding this comment.
This is a bug and should be fixed, but it is not tied to current problem fixed.
| $discriminator = $class::DISCRIMINATOR; | ||
| if (!empty($discriminator) && isset($data->{$discriminator}) && is_string($data->{$discriminator})) { | ||
| $subclass = '\{{invokerPackage}}\Model\\' . $data->{$discriminator}; | ||
| $subclass = '\{{modelPackage}}\\' . $data->{$discriminator}; |
There was a problem hiding this comment.
P2: Resolve discriminator values through the generated mapping before building the subclass name. Custom discriminator mappings will otherwise deserialize to the base model instead of the mapped subtype.
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/ObjectSerializer.mustache, line 502:
<comment>Resolve discriminator values through the generated mapping before building the subclass name. Custom discriminator mappings will otherwise deserialize to the base model instead of the mapped subtype.</comment>
<file context>
@@ -499,7 +499,7 @@ class ObjectSerializer
$discriminator = $class::DISCRIMINATOR;
if (!empty($discriminator) && isset($data->{$discriminator}) && is_string($data->{$discriminator})) {
- $subclass = '\{{invokerPackage}}\Model\\' . $data->{$discriminator};
+ $subclass = '\{{modelPackage}}\\' . $data->{$discriminator};
if (is_subclass_of($subclass, $class)) {
$class = $subclass;
</file context>
There was a problem hiding this comment.
In this scenario (where discriminator is limited to existing class), the use case is valid.
| @@ -0,0 +1,9 @@ | |||
| # # DiscriminatorChild | |||
There was a problem hiding this comment.
P3: Use a single Markdown heading marker for the model title.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/php-nextgen/OpenAPIClient-php/docs/Model/DiscriminatorChild.md, line 1:
<comment>Use a single Markdown heading marker for the model title.</comment>
<file context>
@@ -0,0 +1,9 @@
+# # DiscriminatorChild
+
+## Properties
</file context>
| # # DiscriminatorChild | |
| # DiscriminatorChild |
There was a problem hiding this comment.
the line in template modules/openapi-generator/src/main/resources/php-nextgen/model_doc.mustache
is currently written as:
# {{#models}}{{#model}}# {{classname}}
should be only
# {{classname}}
|
@lukascernydis can you please review the feedback by cubic-dev-ai? |
|
thanks for reviewing the feedback. i'll merge this one and please file a separate PR to address those feedback when you've time. (maven plugin CI failure not related to this change) |
based on #22811 with resolved merge conflicts
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 discriminator subclass detection in
php-nextgenby using the configured model namespace during deserialization to prevent wrong-class instantiation. Adds tests and sample models to cover discriminator behavior.modelPackagenamespace for discriminator subclass lookup inObjectSerializer.testDiscriminatorUsesModelPackageNamespaceto enforce correct namespace (\MyApp\Entities\vs\MyApp\Model\).php-nextgensamples withDiscriminatorBaseandDiscriminatorChildmodels.Written for commit 3f42e23. Summary will update on new commits.