PHP 8.1 attributes #982
PHP 8.1 attributes #982
Conversation
DerManoMann
force-pushed
the
php81-attributes
branch
2 times, most recently
from
c2d3782
to
65fde13
Compare
|
Progress: The openapi-spec example now passed with all analysers! Unfortunately the nested attributes have to be wrapped into a single line to be BC. |
DerManoMann
force-pushed
the
php81-attributes
branch
from
ab6e606
to
22e1ca6
Compare
DerManoMann
force-pushed
the
php81-attributes
branch
2 times, most recently
from
7efbea9
to
bd7775f
Compare
|
From what I see, you want to support both attributes and annotations? Wouldn't it be easier and better to just support attributes in the next major version? |
I think we do have a different understanding of semantic versioning. To me the version bump is justified by the big refactor of existing classes. I doubt that any downstream project using swaggger-php could update to this new code without some work. Having a new major version doesn't mean we do have to abandon all annotation support. Considering that apparently people still use version 2 ;) I would expect lots of projects will have other priorities than updating annotations. The way I see it anyone just using swagger-php via CL should be able to upgrade without pressure of migrating to attributes (which would require updating the project to PHP 8.1, no less). Finally, being able to run both side by side (or mixed, which I think is possible) helps immensly with testing. There are a lot of tests that would not work using the new Beside all that I'd be still interested in actual feedback (testing, things missing, etc...) |
DerManoMann
force-pushed
the
php81-attributes
branch
6 times, most recently
from
2364cac
to
8f54858
Compare
DerManoMann
force-pushed
the
php81-attributes
branch
2 times, most recently
from
30b7e91
to
53add4a
Compare
DerManoMann
force-pushed
the
php81-attributes
branch
from
9f0d667
to
108e7d6
Compare
DerManoMann
force-pushed
the
php81-attributes
branch
from
124b506
to
466320c
Compare
|
Hey everyone! We're using this library in our main project and we're eager to get everything moved over to annotations, so I've started playing with this branch locally. One problem I've found is that my IDE isn't type-hinting the attribute the way it does with the older annotation. I suspect that's because of the conditional class definitions for the parent classes, where they're only defined as attributes if the PHP version is >= 8.0. I'm not sure that there's really any simple workaround for this besides just making a version of the library that requires PHP 8.0 or greater and that leans fully into the attribute support, leaving the annotations for older versions of the library. I'm personally not opposed to that idea at all. |
Thanks for doing some testing - that is much appreciated. What kind of type hinting are you missing exactly? What I do not get (but that is not related to the PHP version) is type hints for the values unless they are strings due to how the library uses |
DerManoMann
force-pushed
the
php81-attributes
branch
4 times, most recently
from
c0a8354
to
1c7334f
Compare
|
@SlvrEagle23 I've added some more type hints to the 8.1 named attributes - could you guy have a look and see if that helped things? Also, those are only enabled with PHP 8.1, not 8.0 as some attributes really require nesting... Anything else, just let me know; I know there are some named properties in the attribute constructors still missing - if you find any please let me know ( or any other issues, for that matter) |
DerManoMann
force-pushed
the
php81-attributes
branch
from
eabfd63
to
65322c9
Compare
|
So, I keep a pretty close eye on the PHP internals and I haven't actually seen anything at all about allowing nested attributes in PHP 8.1, and I looked through the approved RFCs and found nothing of the sort either. It's entirely possible I'm missing something, though. I'd certainly love to see it implemented in 8.1 but I'm not seeing any evidence at the moment that it's coming down the line. |
DerManoMann
force-pushed
the
php81-attributes
branch
2 times, most recently
from
186a20b
to
0defe59
Compare
DerManoMann
force-pushed
the
php81-attributes
branch
4 times, most recently
from
2abc08b
to
780a65a
Compare
Also prepare for usikng the `Generator` in place of the current static config properties
This makes the `Generator` class the authority about whitelisted annotation namespaces. A value of `null` is interpreted as wildcard. Registers a new loader for each `Generator` instance. A new method `withContext()` was also added to allow to run custom code in the context of a configured `Generator`.
Shortcut version of `Parameter(in='path', required: true)`.
In addition to that it can be used as method parameter attribute. In that case the name
and (optiona) type-hint are automatically configured.
Example:
```
public function getProduct(#[OA\PathParameter] string $product_id) {
// ...
}
```
Same as:
```
/**
* @OA\PathParameter(
* name="product_id",
* @OA\Schema(type="string")
* )
*/
```
The legacy `TokenAnalyser` can be used by specifying the `--legacy` (`-l`) option.
DerManoMann
force-pushed
the
php81-attributes
branch
from
94892a6
to
8906ee3
Compare
|
@DerManoMann as reported here, due to the duplication of classes, PHPStorm 2021.2.3 is not able to find the attribute ones
If I comment out the Annotation class and leave the Attribute one, all it's fine:
Wouldn't it be better to define the attributes in a new namespace, for better IDE compat.? |
|
Honestly, I do not know why that would be. I have a project using 4.0.4 attributes and that is fine. I do get a message about multiple implementations but that is it. Again, the configured language level might make a difference. I do agree that a separate namespace would be nice. In fact, that is how I started. Unfortunately, some fo the internals do check for explicit class names to verify allowed parent/child relationships and that would have been a major pain. |
|
@DerManoMann I have the target language as php 8.1. It might be just a phpstorm issue, but anyway having duplicated class names is always tricky because it may get other analysis tools "confused" as well. thanks for the effort in this PR anyway |
|
Thanks @itsjavi; also apologies for avoiding more work :) Happy to say that in light of #1021 I've started looking into a potential split... Turns out some very early iteration had some code to deal with different namespaces for attributes/annotations already - I just thought that never made it into the real PR but it seems I was wrong :) So, chances are the change might not be too bad after all. |
|
Why not 8.0? |
Except for simple attributes there is some nesting required with using |
Sneak preview on attributes (requires PHP 8.1).
In fact, this is now the branch for swagger-php v4!
Feedback welcome!
So far:
TokenAnalyserandAnalyserReflectionAnalyserandAnnotationFactoryInterfaceAnalyserInterfaceimplemented by bothTokenAnalyserandReflectionAnalyserDocBlockAnnotationFactoryandAttributeAnnotationFactory;ReflectionAnalysercan use either (or possibly both at the same time...)DocBlockAnnotationFactory)\Attributetests/Fixtured/Apis/Attributes- this is on parity with the corresponding DocBlock exampleTODO: