api-platform / schema-generator

PHP Model Scaffolding from Schema.org and other RDF vocabularies
https://api-platform.com/docs/schema-generator/
MIT License
458 stars 108 forks source link

entity property default #389

Closed PawelSuwinski closed 2 years ago

PawelSuwinski commented 2 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Adds generating of entity property default value based on #ORM\Column or #ApiProperty attribute as doc generator does.

For example:

# config/schema.yaml
types:
  Book:
    properties:
      condition:
        range: "https://schema.org/OfferItemCondition"
        attributes: 
          ORM\Column: 
            options: 
              default: "https://schema.org/NewCondition"

gives:

// App\Entity\Book
class Book
(...)
   #[Assert\NotNull]
   #[ORM\Column(options: ['default' => 'https://schema.org/NewCondition'])]
    private $condition = 'https://schema.org/NewCondition';

For a background. API property metadata factory take into account default value based on #ORM\Column attribute or #ApiProperty and generates appropriate doc with default keyword. But if this property is send back as not set or it is is out of used normalization group default value will never be persisted as expected (null in database or exception on not null requirement). Doctrine attribute #ORM\Column: { options: { default: (...) } } is not enough because doctrine can't do this (https://www.doctrine-project.org/projects/doctrine-orm/en/latest/reference/faq.html#how-can-i-add-default-values-to-a-column). Default value should be explicitly set in entity class during property declaration but schema-generator does not generate this defaults. This PR fix it.

alanpoulain commented 2 years ago

I don't think it is the right fix, it is too much magical and very specific to API Platform / Doctrine. What do you think of having a defaultValue parameter in the property configuration instead?

PawelSuwinski commented 2 years ago

I don't think it is the right fix, it is too much magical and very specific to API Platform / Doctrine. What do you think of having a defaultValue parameter in the property configuration instead?

I wanted to change the least code possible to get working consistent solution, but I get the point. So here it is an implementation with proposed defaultValue property configuration.

For example:

# config/schema.yaml
types:
  BookFormatType: ~
  Book:
    properties:
      isbn: ~
      bookFormat:
        range: "BookFormatType"
        defaultValue: "https://schema.org/Hardcover"

gives:

(...)
    /**
     * The format of the book.
     *
     * @see https://schema.org/bookFormat
     */
    #[ORM\Column(nullable: true, options: ['default' => 'https://schema.org/Hardcover'])]
    #[ApiProperty(types: ['https://schema.org/bookFormat'], default: 'https://schema.org/Hardcover')]
    #[Assert\Choice(callback: [BookFormatType::class, 'toArray'])]
    private ?string $bookFormat = 'https://schema.org/Hardcover';
alanpoulain commented 2 years ago

Yes, I think it would be great like this! I'm not convinced about adding the default value to ORM\Column and ApiProperty attributes though.

PawelSuwinski commented 2 years ago

I'm not convinced about adding the default value to ORM\Column and ApiProperty attributes though.

It seemed natural as was suggested by DoctrineOrmPropertyMetadaFactory that already use it to set default in generated api doc: https://github.com/api-platform/core/blob/a16cb35ec3619f2d63471318468eb437515a7f90/src/Doctrine/Orm/Metadata/Property/DoctrineOrmPropertyMetadataFactory.php#L77

But I don't insist on that. Let me know if I should withdraw this part from PR.

alanpoulain commented 2 years ago

Yes, I think it should only change the PHP default value.

PawelSuwinski commented 2 years ago

Done.

PawelSuwinski commented 2 years ago

OK, we can take less strict approach moving responsibility to user. So here it is an implementation without validation.

I looks like some commits added in meanwhile doesn't pass the phpstan build check (?) (only php7.4, php8.1 does).

alanpoulain commented 2 years ago

Thank you @PawelSuwinski!