dustin10 / VichUploaderBundle

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

Explicit that nullable is a necessity #1379

Closed mysterty closed 1 year ago

mysterty commented 1 year ago

I just experienced this issue. I didn't thinck that "nullable" was REALLY needed, as i was using my own uploading before and set my field as "uniq". Maybe i wont be the only person experiencing it so… i guess it cost nothing to precise it.

more here : https://stackoverflow.com/questions/75861160/symfony-6-vichuploader-cant-update-delete-file/75902101#75902101

garak commented 1 year ago

Thank you for your contribution. In any Doctrine mapping, every property that has a nullable type must be declared as nullable. This is not related to this bundle, but to the way Doctrine manages the mapping itself.

So, I think it should be understood by anyone, and anyway the documentation is already proposing the correct way.

mysterty commented 1 year ago

It's not obvious that the property needs to be declared nullable.

As i read the doc, it was just for the exemple that the property is nullable.

For database integrity, we could conciderate that the upload field needs to be not null. Indeed, an uploadable entity without data attached should be destroyed…

garak commented 1 year ago

If you declare a property with a default value of null (like in the example you linked), you must declare it as nullable. Maybe it's not obvious (one could argue that Doctrine should be able to understand it), but it's how Doctrine works.

mysterty commented 1 year ago

In fact, it's doctrine normal functionment. Php declaration is null even if the SQL schema property is not nullable (and the symfony maker create not nullable entities with null default value).

So, it's clearly needed to explicit that doctrine property's declaration need to be "Nullable" and it's not "just for exemple".

(Plus i just tried to force php property to really being not nullable -handle some bugs as it's not normal declaration- and it still doesn't be handled by vich uploader)

garak commented 1 year ago

It's not needed to be explicit: the null default value is already provided by the example (and even by generated code, in the case of the maker). The nullable mapping is a direct consequence of the property being null, and not setting it violates the Doctrine standard, not a rule set in this bundle.

mysterty commented 1 year ago

In absolute, i would agree with you. But symfony allow to map a doctrine not nullable property with a nullable php property AND do it by default

So, concidering this fact, the actual documentation is ambigous.

garak commented 1 year ago

In absolute, i would agree with you. But symfony allow to map a doctrine not nullable property with a nullable php property AND do it by default

If so, this is a bug in Symfony itself (I guess in the maker bundle)