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

Fix nullable detection #6276

Closed
wants to merge 2 commits into from
Closed

Conversation

DrWarpMan
Copy link
Contributor

@DrWarpMan DrWarpMan commented Apr 22, 2024

In Symfony production environment, since Doctrine 3.x, cached values for 'nullable' property can be either null, false or true. In Doctrine 2.x, it's only false or true. When Doctrine returns null for the nullable property, the required option for the field is set to false.

While in fact, Doctrine considers nullable => null as nullable => false, thus the field should be required.

To reproduce:

  • create an entity, with a field that has 'nullable' property set to false
  • create an admin controller for the entity (make sure to NOT set setRequired(true) explicitly)
  • try to create a new record in the dashboard, you will see that the field is optional

References:
Doctrine 3.x $nullable
How Doctrine determines nullable

In Symfony production environment, since Doctrine 3.x, values for 'nullable' property can be either null, false or true. In Doctrine 2.x, it was only false or true. When Doctrine returned 'null' for the nullable property, the required option for the field was set to false.

While in fact, Doctrine considers 'nullable => null' as 'nullable => false', thus should make the field required.
@ServerExe
Copy link

This is some kind of strange behavior of Doctrine or EasyAdmin. But changing the APP_ENV to prod and removing the var/cache folder leads to exactly this behavior as it is on production. When debugging, it's really hard to find the turning point in the code where it takes another path of workflow. I am not sure but somehow it might have something to do with Doctrine Caching system. Because on production mode it doesn't even call the AdminContextFactory, where actually some metadata would be loaded from the entities. I am really not sure if my information is valid but as it seems production has a completely different behavior. I realized that when loading the PHP attributes of the entity via Reflection it recognizes the #[ORM\Id] and #[ORM\Column] attributes on DEV mode but it completely ignores them on production mode. So the default KeyValueStore of these attributes aren't even loaded ...

I can't rely on the entity property detection anymore and will add the required flag to all of my fields in the crud controllers :(

@javiereguiluz javiereguiluz added this to the 4.x milestone Jun 26, 2024
@javiereguiluz
Copy link
Collaborator

Thanks @DrWarpMan for fixing this bug! And sorry it took us so long to merge it.

javiereguiluz added a commit that referenced this pull request Jun 26, 2024
This PR was squashed before being merged into the 4.x branch.

Discussion
----------

Fix nullable detection

In Symfony production environment, since Doctrine 3.x, **cached** values for 'nullable' property can be either `null`, `false` or `true`. In Doctrine 2.x, it's only `false` or `true`. When Doctrine returns `null` for the nullable property, the required option for the field is set to `false`.

While in fact, Doctrine considers `nullable => null` as `nullable => false`, thus the field should be required.

To reproduce:
- create an entity, with a field that has 'nullable' property set to `false`
- create an admin controller for the entity (make sure to **NOT** set `setRequired(true)` explicitly)
- try to create a new record in the dashboard, you will see that the field is optional

References:
[Doctrine 3.x $nullable](https://github.com/doctrine/orm/blob/f79d166a4e844beb9389f23bdb44abdbf58cec38/src/Mapping/FieldMapping.php#L25)
[How Doctrine determines nullable](https://github.com/doctrine/orm/blob/c3cc0fdd8c9ffb102170c930ca56188626a34719/src/Mapping/ClassMetadata.php#L1030)

<!--
Thanks for your contribution! If you are proposing a new feature that is complex,
please open an issue first so we can discuss about it.

Note: all your contributions adhere implicitly to the MIT license
-->

Commits
-------

e500276 Fix nullable detection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants