dustin10 / VichUploaderBundle

A simple Symfony bundle to ease file uploads with ORM entities and ODM documents.
MIT License
1.85k stars 519 forks source link

Why you make all attribute metadata classes final? How to make predefined options? #1349

Closed mrFleshka closed 1 year ago

mrFleshka commented 1 year ago
Q A
Bundle version 2.0.1
Symfony version 6.1.5
PHP version 8.1.4

Support Question

Hi, i try to remove duplicate code from attributes metadata on every properties like:

...
    #[Vich\UploadableField(mapping: "seo_og_image", fileNameProperty: File::FIELD_NAME, size: File::FIELD_SIZE, mimeType: File::FIELD_MIME_TYPE, dimensions: File::FIELD_DIMENSIONS)]
    protected ?SymfonyFile $file = null;
...

Before version 2 i extend UploadableField class and create new class for attributes with predefined values on constrictor:

...
    #[BaseUploadableField(mapping: "seo_og_image")]
    protected ?SymfonyFile $file = null;
...

Now class is final and i have no idea how to set this attribute parameters by default... Any idea?

garak commented 1 year ago

The class was marked as "internal" since July 2021, so the extension was not supported. I guess you can create a copy of the class instead of extending it.

mrFleshka commented 1 year ago

The class was marked as "internal" since July 2021, so the extension was not supported. I guess you can create a copy of the class instead of extending it.

Yeah i know that it was marked final in annotations yet, but this is attribute and attributes reader try to find only this class, not the copy:

vich/uploader-bundle/src/Metadata/Driver/AnnotationDriver.php

...
        foreach ($properties as $property) {
            $uploadableField = $this->reader->getPropertyAnnotation($property, UploadableField::class);
            if (null === $uploadableField) {
                continue;
            }
...

My goal it not extend it at all, but have mechanism to set some params as default values. As i see from code, its impossible....

I mean fileNameProperty, size, mimeType and dimensions

garak commented 1 year ago

So what did you do previously and you can't do anymore?

mrFleshka commented 1 year ago

So what did you do previously and you can't do anymore?

Previously i just extend my own BaseUploadableField from UploadableField and set properties with values by default (see my first post). It was on PHP 7.2 and class UploadableField not been final yet

Now i copy all options to all attributes where i use #[Vich\UploadableField and it looks ugly...

garak commented 1 year ago

Now i copy all options to all attributes where i use #[Vich\UploadableField and it looks ugly...

So you can do it in the end. Your problem is that your class looks ugly?

mrFleshka commented 1 year ago

So you can do it in the end. Your problem is that your class looks ugly?

Not class but field attribute data. In my project we use STI to store all files (one table with only one parameters for all files). For all properties in entities where i use Vich uploader now we write full line with all params (but really we need only mapping value, other must be setted by default). It breaks my perfect world and add negativ copy-pasting (we have a lot of files fields)...

garak commented 1 year ago

Why are you forced to use all parameters?

mrFleshka commented 1 year ago

Why are you forced to use all parameters?

Because i need this params to store it data in my entity isn't that how it works?

garak commented 1 year ago

OK now I got it. Did you consider the option of using an embedded object?

mrFleshka commented 1 year ago

Did you consider the option of using an embedded object?

No, it poorly documented and i have my own File entity for whore project. Also in different mappings we have different paths and custom directory namers. Don't know how it will work with embedded object.

garak commented 1 year ago

Well, embedded objects may be poorly documented, but they are more documented than your solution. And, even more important, they have official support.

mrFleshka commented 1 year ago

Well, embedded objects may be poorly documented, but they are more documented than your solution. And, even more important, they have official support.

I'm sorry, but you didn't understand my question, I guess. I don't need embedded at all. I have relations for this.

garak commented 1 year ago

You said your question is trying "to remove duplicate code from attributes metadata on every properties". An embedded can help on that.

mrFleshka commented 1 year ago

You said your question is trying "to remove duplicate code from attributes metadata on every properties". An embedded can help on that.

No, it cant.

So, i'll try to explain my structure, but i have now solution for my first question. A lot of code was removed from examples to display only attributes and entities structure.

I have my own abstract entity File:

#[ORM\Entity(repositoryClass: FileRepository::class)]
#[ORM\HasLifecycleCallbacks]
#[ORM\InheritanceType("SINGLE_TABLE")]
#[ORM\DiscriminatorColumn(name: "type", type: Types::INTEGER)]
#[ORM\DiscriminatorMap([
    self::TYPE_OG_IMAGE => OgImage::class,
    //Other types
])]
abstract class File
{
    const TYPE_OG_IMAGE = 1;
    //Other types

    #[ORM\Column(type: Types::INTEGER)]
    #[ORM\Id]
    #[ORM\GeneratedValue]
    protected int $id;

    #[ORM\Column(type: Types::STRING, length: 255, nullable: true)]
    public ?string $name = null;

    #[ORM\Column(type: Types::INTEGER, options: ['unsigned' => true, 'default' => 0])]
    protected ?int $size = 0;

    #[ORM\Column(type: Types::STRING, length: 100)]
    protected ?string $mimeType;

    #[ORM\Column(type: Types::DATETIME_IMMUTABLE)]
    protected DateTimeImmutable $createdAt;

    #[ORM\Column(type: Types::DATETIME_IMMUTABLE)]
    protected DateTimeImmutable $updatedAt;

    #[ORM\Column(type: Types::SIMPLE_ARRAY, nullable: true)]
    protected ?array $dimensions = null;

    #[ORM\Column(type: Types::JSON, nullable: true)]
    protected ?array $pathData = null;

    protected ?SymfonyFile $file;
}

That describes base table in DB for storing all files in my application. Then for every file type i write real entity based on this abstract one:

#[ORM\Entity(repositoryClass: OgImageRepository::class)]
#[Vich\Uploadable]
class OgImage extends File
{
    #[ORM\OneToOne(mappedBy: SeoInfo::FIELD_OG_IMAGE, targetEntity: SeoInfo::class)]
    protected ?SeoInfo $seoInfo = null;

    #[Assert\Image(
        mimeTypes: ["image/jpeg", "image/pjpeg"],
        minWidth: 1200,
        maxWidth: 1200,
        maxHeight: 628,
        minHeight: 628,
        mimeTypesMessage: "Для загрузки доступны только JPG!"
    )]
    #[Vich\UploadableField(mapping: "seo_og_image", fileNameProperty: File::FIELD_NAME, size: File::FIELD_SIZE, mimeType: File::FIELD_MIME_TYPE, dimensions: File::FIELD_DIMENSIONS)]
    protected ?SymfonyFile $file = null;
}

Then i have entity that have relation with this image (as you can see above in OneToOne mapping):

#[ORM\Entity(repositoryClass: SeoInfoRepository::class)]
class SeoInfo extends BaseTranslatableEntity
{
    #[ORM\OneToOne(inversedBy: OgImage::FIELD_SEO_INFO, targetEntity: OgImage::class, cascade: ['persist', 'remove'])]
    #[ORM\JoinColumn(onDelete: "SET NULL")]
    protected ?OgImage $ogImage = null;
}

I don't need embedded for this structure because i already have all my fields for image described in abstract entity class.

Then i need to display in attribute for VichUploadable entity all params as you can see above:

    #[Vich\UploadableField(mapping: "seo_og_image", fileNameProperty: File::FIELD_NAME, size: File::FIELD_SIZE, mimeType: File::FIELD_MIME_TYPE, dimensions: File::FIELD_DIMENSIONS)]
    protected ?SymfonyFile $file = null;

This attribute MUST be in all my files entities and contain ALL described params: fileNameProperty, size, mimeType, dimensions. But in reality i need to specify only mapping, because it unique.

All I wanted was to be able to specify only mapping for this:

    #[Vich\UploadableField(mapping: "seo_og_image")]
    protected ?SymfonyFile $file = null;

But other parameters must have default values like

#[\Attribute(\Attribute::TARGET_PROPERTY)]
class UploadableField implements AnnotationInterface
{
    /**
     * Constructs a new instance of UploadableField.
     */
    public function __construct(
        private readonly string $mapping,
        private readonly ?string $fileNameProperty = File::FIELD_NAME,
        private readonly ?string $size = File::FIELD_SIZE,
        private readonly ?string $mimeType = File::FIELD_MIME_TYPE,
        private readonly ?string $originalName = null,
        private readonly ?string $dimensions = File::FIELD_DIMENSIONS,
    ) {
    }
}

To achieve this in previous version i just extend UploadableField class and set this default params as previous example. Then i create my entity and mark file field with my own Attribute:

    #[BaseUploadableField(mapping: "seo_og_image")]
    protected ?SymfonyFile $file = null;

And other UploadableField parameters was settled by default.

Because your AnnotationDriver try to find only UploadableField classes in attributes it worked (as extended from UploadableField)

AnnotationDriver.php

...
        foreach ($properties as $property) {
            $uploadableField = $this->reader->getPropertyAnnotation($property, UploadableField::class);
            if (null === $uploadableField) {
                continue;
            }
...

So. i think i have provided enough information for understanding my solution.

Now more about Embedded. In your example in docs you don't have one line with attribute mapping for example how to use embedded:

#[ORM\Entity]
#[Vich\Uploadable]
class Product
{
    #[ORM\Id]
    #[ORM\Column(type: 'integer')]
    #[ORM\GeneratedValue(strategy: 'AUTO')]
    private ?int $id = null;

    // ... other fields

    #[ORM\Embedded(class: "Vich\UploaderBundle\Entity\File")]
    private ?EmbeddedFile $image = null;

    #[ORM\Column(type: 'datetime')]
    private ?\DateTimeInterface $updatedAt = null;

This part from docs and this how it must look:

#[ORM\Entity]
#[Vich\Uploadable]
class Product
{
    #[ORM\Id]
    #[ORM\Column(type: 'integer')]
    #[ORM\GeneratedValue(strategy: 'AUTO')]
    private ?int $id = null;

    // ... other fields

    // NOTE: This is not a mapped field of entity metadata, just a simple property.
    #[Vich\UploadableField(mapping: 'products', fileNameProperty: 'imageName', size: 'imageSize')]
    private ?File $imageFile = null;

    #[ORM\Embedded(class: "Vich\UploaderBundle\Entity\File")]
    private ?EmbeddedFile $image = null;

    #[ORM\Column(type: 'datetime')]
    private ?\DateTimeInterface $updatedAt = null;

And as you can see you must set up all options in all examples for UploadableField if you need to store it (fileNameProperty, size etc.). All what give you Embedded is simplify fields in entity.


Now about my solution. If anyone have this problems too. It really ugly but i love how my structure work previously and ready to make this.

We need to create copy of UploadableField in you project (not in src dir!) for example in vich/Annotations/UploadableField.php

And remove final (or set default values as i did)

Then we need to change autoload in composer.json:

    "autoload": {
        "psr-4": {
            "App\\": "src/"
        },
        "exclude-from-classmap": ["vendor/vich/uploader-bundle/src/Mapping/Annotation/UploadableField.php"],
        "files": ["vich/Annotations//UploadableField.php"]
    },

And don't forget to run composer dump-autoload That's it. Now only your own UploadableField class will work and you can extend it to set other default fields.

Of course this may be broken in the future depending on plugin changes =)

garak commented 1 year ago

Of course this may be broken in the future depending on plugin changes =)

This is exactly the purpose of having final classes in a library: moving the burden of unsupported behaviours to users. This allows the library maintainers to focus on supported behaviours.

If you think that your use case can be of general interest and that it can be implemented in a configurable way, I'll be happy to review a pull request.

Meanwhile, since it looks like you hate repeating configurations, I can give a suggestion: you can omit 90% of your Doctrine mapping types (e.g. Types::STRING), they're inferred from property types.

mrFleshka commented 1 year ago

If you think that your use case can be of general interest and that it can be implemented in a configurable way, I'll be happy to review a pull request.

This case is bad, and i know it. If you set classes final than you really need it (i think). Maybe it will work with UploadableFieldInterface in AnnotationDriver... I'll check it and try to create PR in future.

Meanwhile, since it looks like you hate repeating configurations, I can give a suggestion: you can omit 90% of your Doctrine mapping types (e.g. Types::STRING), they're inferred from property types.

Oh, you got me... But i specially set type for Doctrine Mapping all times (even if it string), because it looks obvious. And i really hate repeat completely unnecessary configurations...

mrFleshka commented 1 year ago

And what about your docs? Was i right about the clarification?

garak commented 1 year ago

And what about your docs? Was i right about the clarification?

Yes, the embedded class proposed in the docs is not mapped. You can embed your own class anyway and map it, but you need to map also the main class.