zircote
Wrong docs generation for php8 promoted properties.
Using php 8 promoted properties, I expect the doccomment to be parsed the same as for the basic class properties. But it actually looks like they are overridden by method arguments. As far as I'm concerned, the only reason I want to use elevated properties is the linter hints (and one more, but it's affected by the class extension). I tried to investigate on my own but was unsuccessful. Maybe someone also wants to use PP like me and this could be a nice fix. In my opinion, the problem is overriding because ReflectionClass->getProperties() returns two values with the correct doc comments.
Code:
#[Schema()]
class NameValue
{
/**
* Property name.
*
* @var string
*/
#[Property()]
public string $name = '';
public function __construct(
/**
* Property value.
*
* @var string
*/
#[Property()]
public string $value = '',
) {
}
}
Expected result:
"NameValue": {
"properties": {
"value": {
"description": "Property value.",
"type": "string",
"nullable": false
},
"name": {
"description": "Property name.",
"type": "string"
}
},
"type": "object"
},
Actual result:
"NameValue": {
"properties": {
"value": {
"type": "string",
"nullable": false
},
"name": {
"description": "Property name.",
"type": "string"
}
},
"type": "object"
},
Yep, that could be better.
Also the "nullable": false is kinda redundant I suppose...
And also one more bug founded here. While there is no fix yet, I tried to move all the comments to property description attribute parameter, and found that property property ignored.
Class:
#[Schema()]
class NameValue
{
/**
* Property name.
*
* @var string
*/
#[Property(property: 'name_override')]
public string $name = '';
public function __construct(
/**
* Property value.
*
* @var string
*/
#[Property(property: 'value_override')]
public string $value = '',
) {
}
}
Expected result:
"NameValue": {
"properties": {
"value_override": {
"type": "string",
"nullable": false
},
"name_override": {
"description": "Property name.",
"type": "string"
}
},
"type": "object"
},
Actual result:
"NameValue": {
"properties": {
"value": {
"type": "string",
"nullable": false
},
"name_override": {
"description": "Property name.",
"type": "string"
}
},
"type": "object"
},
As for me, I'd just put construct to ignored methods, but this may affected another functionality. Any ideas?
@DerManoMann don't know exactly how it works, but should you manually update packagist, or it will be updated later by itself?
@DerManoMann Updated from version 4.5.1 to 4.7.2, I got:
- Enum fields now takes keys instead of values (I think its a bit weird)
- Bug below:
#[Schema()]
class NameValue
{
public function __construct(
/**
* Property name.
*
* @var string
*/
#[Property()]
public string $name = '',
/**
* Property value.
*
* @var string
*/
#[Property()]
public string $value = '',
) {
}
}
Actual result:
"NameValue": {
"properties": {
"name": {
"description": "Property value.",
"type": "string"
},
"value": {
"type": "string"
}
},
"type": "object"
},
Expected result:
"NameValue": {
"properties": {
"name": {
"description": "Property name.",
"type": "string"
},
"value": {
"description": "Property value.",
"type": "string"
}
},
"type": "object"
},
@DerManoMann please, reopen this issue, because there is still bug with description, that I've mentioned above.
If you could provide a new isolated example that would be great - this issue is getting a bit crowded
Unfortunately I was failed to create fully isolated project, but the problem could be reproduced just with one model.
<?php
use OpenApi\Attributes\Property;
use OpenApi\Attributes\Schema;
#[Schema()]
class NameValue
{
public function __construct(
/**
* Property name.
*
* @var string
*/
#[Property()]
public string $name = '',
/**
* Property value.
*
* @var string
*/
#[Property()]
public string $value = '',
) {
}
}
In docs, for this model, comments will be messed up.
If I can help in any other way, please, let me know.
Cool. I think I can see what is going on. Unfortunately, the fix is tricky as it randomly breaks other stuff :/ We'll see how it goes...
Any news on this? I am facing the same issue.
Also, please let me know if I can help in any way
I'm pretty sure I'm encountering this same issue, and have a working theory as to why it happens.
In ReflectionAnalyzer:129 it loops through methods, and creates a context for that method. It then invokes AttributeAnnotationFactory@build which will loop through the method arguments, and attach docblocks to these.
https://github.com/zircote/swagger-php/blob/ce3b9be639918cc0b1eed5e24869a72f8a2ca357/src/Analysers/ReflectionAnalyser.php#L129-L142
It just so happens, that __construct() is also a method, and in the case it of promoted properties it will build the properties through the constructor arguments, instead of through the properties themselves. There is a hint in AttributeAnnotationFactory:34`:
https://github.com/zircote/swagger-php/blob/ce3b9be639918cc0b1eed5e24869a72f8a2ca357/src/Analysers/AttributeAnnotationFactory.php#L34-L37
The problem is, that normally each property will have its own context. But because promoted properties are handled as method arguments, they all share the same method context. So for each promoted property, it actually just overwrites the previous docblock, and only the last one will be used.
As a proof of concept, I added the following line on AttributeAnnotationFactory:83, and the bug indeed disappeared.
if ($rp->isPromoted()) {
$instance->_context = clone $instance->_context; // <-- The fix
Whether this is a good fix or not, I'm not sure. But at least I hope it sheds some light on the issue.
I'm hoping someone with more knowledge on the inner workings will help create a proper bug fix.
Its the right idea, however cloning context is sometimes tricky. I do wonder if yours is still a bit different from the original issue as there is now a testcase that should cover this issue and your suggeted change does not change the behaviour at all for that: https://github.com/zircote/swagger-php/blob/master/tests/Fixtures/Scratch/PromotedProperty.php
If your case is different I'd be interested in seeing a sample so I can add another test for that case too.
Its the right idea, however cloning context is sometimes tricky. I do wonder if yours is still a bit different from the original issue as there is now a testcase that should cover this issue and your suggeted change does not change the behaviour at all for that: https://github.com/zircote/swagger-php/blob/master/tests/Fixtures/Scratch/PromotedProperty.php
If your case is different I'd be interested in seeing a sample so I can add another test for that case too.
Thanks @DerManoMann - I just created a PR which reproduces and fixes the issue I described. Interested in hearing your thoughts.
@rasmuscnielsen Thanks for your PR.
For my case that fixed. But got one more (probably new one)
#[Schema()]
class NameValue
{
public function __construct(
/**
* Property name.
*
* @var string
*/
#[Property()]
public string $name = '',
/**
* Property value.
*
* @var string
*/
#[Property()]
public string $value = '',
) {
}
}
Expected result:
"NameValue": {
"properties": {
"name": {
"description": "Property name.",
"type": "string"
},
"value": {
"description": "Property value.",
"type": "string"
}
},
"type": "object"
}
Actual result:
"NameValue": {
"properties": {
"name": {
"description": "Property name.",
"type": "string"
},
"value": {
"type": "string"
}
},
"type": "object"
}
So the value description is missing.
UPD: After checking other DTOs, I see that no matter how many properties I have, the comment will be saved just for the first one.
Oh, yes, that fails. Very strange. Thanks for the reproducer - will have a look later.
Ah, the scratch test used an attribute and then an annotation - two attributes in a row does indeen still fail.