Allow generating documentation when property and method start from "is"

[sidz]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
License	MIT

Currently API Platform doesn't generate documentation for case when both property and method names start from is. We need to define isIsActive or getIsActive or canIsActive or hasIsActive

For example:

<?php

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Serializer\Annotation\Groups;

/**
 * @ApiResource(
 *     normalizationContext={"groups"={"read_user"}, "skip_null_values"=false},
 *     denormalizationContext={"groups"={"create_user"}},
 *     itemOperations={},
 *     collectionOperations={
 *         "get"= {
 *             "normalization_context"={"groups"={"read_user"}, "skip_null_values"=false},
 *         },
 *         "post"={
 *             "validation_groups"={"user:create"}
 *         }
 *     }
 * )
 *
 * @ORM\Entity
 */
class User
{
    /**
     * @ORM\Id
     * @ORM\GeneratedValue
     * @ORM\Column(type="integer", options={"unsigned"=true})
     *
     * @Groups({"read_user"})
     */
    private ?int $id = null;

    /**
     * @ORM\Column(type="string", length=50)
     *
     * @Groups({"read_user", "create_user"})
     */
    private string $name;

    /**
     * @ORM\Column(type="boolean")
     *
     * @Groups({"read_user", "create_user"})
     */
    private bool $isActive;

    public function __construct(string $name, bool $isActive)
    {
        $this->name = $name;
        $this->isActive = $isActive;
    }

    public function getId(): ?int
    {
        return $this->id;
    }

    public function getName(): string
    {
        return $this->name;
    }
    
    public function isActive(): bool
    {
        return $this->isActive;
    }
}

In this case documentation won't include property isActive. But if we add missed property to the request it will be processed successfully and entity will be created.

Documentation for such case is not generated due to changes in symfony since 5.1 inside ReflectionExtractor: https://github.com/symfony/symfony/blob/v5.1.0/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L232-L253

Let's see on line https://github.com/symfony/symfony/blob/v5.1.0/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L239-L240

Property $camelProp contains IsActive string which will be iteratively concatenated with all possible prefixes. We get 4 possible method names which we may have to solve this issue:

• isIsActive
• canIsActive
• getIsActive
• hasIsActive

But we don't have such method in our class and we would like to use isActive without prefixes.
So we go to line https://github.com/symfony/symfony/blob/v5.1.0/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L249

there we can see that $reflClass->hasMethod($getsetter) will find method isActive in our class but $allowGetterSetter is equal to false as $context['enable_getter_setter_extraction'] is null.

So this PR solve this issue and it will be possible to have isActive property and the same method name.

p.s. The answer on question why everything works fine when we add missed isActive property to the request manually is on lines: https://github.com/symfony/property-access/blob/v5.1.0/PropertyAccessor.php#L472-L476

[alanpoulain]

IMO you could add it directly above 🙂

[sidz]

yeah I thought about this as well.
But IMHO it won't look good in case when it will be needed to add one more key-value there with explanation comment.

[alanpoulain]

Why not like this:

$options = [
    /* @see https://github.com/symfony/symfony/blob/v5.1.0/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php */
    'enable_getter_setter_extraction' => true,
    // Another explanation.
    'another_option' => 'foo',
];

[sidz]

I'm okay to do this if you still think that it is critical. Should I?

[alanpoulain]

It's not critical at all. It's only some code style 🙂

[sidz]

done in this case 👍

[alanpoulain]

Is it wanted?

[sidz]

nope. reverted

[alanpoulain]

Couldn't it be like a BC? Since the generated OpenAPI documentation will change after this PR?
Shouldn't it be better to target main?

[sidz]

@alanpoulain I'd say that it is not a BC as API Platform just doesn't generate right documentation.

without this fix and exactly same structure like in my example class I'd be able to add isActive property manually click on Execute button and user will be created/updated.

[alanpoulain]

Makes sense indeed.

[alanpoulain]

Thank you @sidz and congratulations for your first contribution (it seems)!

[sidz]

thank you @alanpoulain, @dunglas for assistance

[soyuka]

Changing existing tests is not recommended, I hope that using isDummyBoolean on the property still works.

[alanpoulain]

The isDummyBoolean method was not taken for the schema generation, only the property.
I don't think the tests have changed.