Skip to content
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

[attributes] add support for class constants #1147

Closed
wants to merge 3 commits into from

Conversation

cdaguerre
Copy link
Contributor

@cdaguerre cdaguerre commented Mar 1, 2022

This PR :

1. Attributes on class constants

use OpenApi\Attributes as OA;

#[OA\Schema()]
class Airport
{
    #[OA\Property(property='kind')]
    public const KIND = 'Airport';
}

2. Support the const keyword in attributes

use OpenApi\Attributes as OA;

#[OA\Schema()]
class Airport
{
    #[OA\Property(
        property: 'kind',
        type: 'const',
        const: 'Airport'
    )]
    public string $kind = 'Airport';
}

This ^^code is equivalent to the example above in terms of generated documentation.

3. Backport const type properties for versions < 3.1

The const keyword was introduced in OAS 3.1, eg.

components:
  schemas:
    Airport:
        properties:
          kind:
            type: const
            const: Airport

In OAS < 3.1, the recommended way of doing this was to declare a single-valued enum instead, eg.

components:
  schemas:
    Airport:
        properties:
          kind:
            type: enum
            enum: 
              - Airport

I added a processor that will transform const to enum when the given OpenApi version is < 3.1.

Copy link
Collaborator

@DerManoMann DerManoMann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesone PR! Thanks for taking the time to be so thorough.

If I could perhaps ask for a couple more (small) things:

  • Add a little test
  • I wonder if your great PR description could be used to add to the docs somewhere?
    For now the cookbook might be best I think. (If not that is also fine, I can do that later)

Other than that this looks good to go once the question about type = 'const' is sorted.

@cdaguerre
Copy link
Contributor Author

@DerManoMann whilst trying to add a test I found out that class constant annotations are surprisingly not supported by doctrine/annotations (yet)... (ref. doctrine/annotations#263)
Also, there seem to be no tests for attributes and I'm not sure where to go from here.
I think I still makes sense to support class constants with attributes even if they're not supported with annotations but I'd like to have your opinion on this.

@DerManoMann
Copy link
Collaborator

class constant annotations are surprisingly not supported ...

Hmm, not sure that is relevant here as we pull docblocks ourself and stuff them into doctrine.
To support annotations (and generally, really, sorry I didn't spot that earlier) the code should move from the attribute factory into the ReflectionAnalyser.

I had a quick play around and ended up with this near the bottom of analyzeFqdn():

        foreach ($rc->getReflectionConstants() as $constant) {
            foreach ($this->annotationFactories as $annotationFactory) {
                $definition['constants'][$constant->getName()] = $ctx = new Context([
                    'constant' => $constant->getName(),
                    'comment' => $constant->getDocComment() ?: Generator::UNDEFINED,
                    'annotations' => [],
                ], $context);
                foreach ($annotationFactory->build($constant, $ctx) as $annotation) {
                    if ($annotation instanceof Property) {
                        if (Generator::isDefault($annotation->property)) {
                            $annotation->property = $constant->getName();
                        }
                        if (Generator::isDefault($annotation->const)) {
                            $annotation->const = $constant->getValue();
                        }
                        $analysis->addAnnotation($annotation, $ctx);
                    }
                }
            }
        }

@DerManoMann
Copy link
Collaborator

For testing you could add a const to the three basic.php under tests/Fixtures/Apis/xxx and update the shared basic.yaml accordingly.

@DerManoMann
Copy link
Collaborator

@cdaguerre Are you still going to work on this? Happy to clean up if it feels like it is too much.

@cdaguerre
Copy link
Contributor Author

@DerManoMann it'd be great if you could take over ;)

@DerManoMann
Copy link
Collaborator

No problem. It would be a shame to throw away your work.

@DerManoMann
Copy link
Collaborator

Replaced by #1193

@DerManoMann DerManoMann closed this Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants