-
Notifications
You must be signed in to change notification settings - Fork 932
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
Wrong docs generation for php8 promoted properties. #1419
Comments
Yep, that could be better. Also the |
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 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? |
Good point! I forgot. |
@DerManoMann Updated from version 4.5.1 to 4.7.2, I got:
#[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 :/ |
Any news on this? I am facing the same issue. Also, please let me know if I can help in any way |
Not really, sorry. |
I'm pretty sure I'm encountering this same issue, and have a working theory as to why it happens. In swagger-php/src/Analysers/ReflectionAnalyser.php Lines 129 to 142 in ce3b9be
It just so happens, that swagger-php/src/Analysers/AttributeAnnotationFactory.php Lines 34 to 37 in ce3b9be
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
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. 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. |
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:
Expected result:
Actual result:
The text was updated successfully, but these errors were encountered: