Conversation
MKodde
left a comment
MKodde
left a comment
There was a problem hiding this comment.
Nice work! We had a small discussion IRL where I questioned the current solution:
- Could you make a more 'generic' solution for the object and array types that you now added. This would need to include passing the type of the array items that are stored for additional type checking. And maybe passing a max length as some types have different requirements for that..
- Keep the current setup but add some additional (unit or integration) tests. I've found a couple of unit tests for two of the existing types
Question: are the types used in the existing functional tests? Or are they bypassed by the custom mock solution?
| * @var string[] | ||
| */ | ||
| #[ORM\Column(name: 'allowed_idp_entity_ids', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 6777215)] | ||
| #[ORM\Column(name: 'allowed_idp_entity_ids', type: 'json', length: 16777215, nullable: true)] |
There was a problem hiding this comment.
Was the (significant) length increase intentional?
I also see that the field became nullable, but that is now explicitly set for all of the updated columns. I'm assuming that was previously the default setting which now needs an explicit setting.
There was a problem hiding this comment.
@johanib Note to self: Do we really want this? Better to avoid a migration and make specific types for the fields that use array, or copy the Array type into EB.
There was a problem hiding this comment.
Error: Use SimpleArray, json is fundamentally different and requires a migration.
There was a problem hiding this comment.
@MKodde I checked the length with: #1861 (comment)
Its currently mediumtext on prod, so it should be 16777215. I htink 6777215 was a typo.
johanib
force-pushed
the
feature/update_dbal_4_1799_3
branch
2 times, most recently
from
805c922 to
271256a
Compare
| * @var string[] | ||
| */ | ||
| #[ORM\Column(name: 'allowed_idp_entity_ids', type: \Doctrine\DBAL\Types\Types::ARRAY, length: 6777215)] | ||
| #[ORM\Column(name: 'allowed_idp_entity_ids', type: 'json', length: 16777215, nullable: true)] |
There was a problem hiding this comment.
Error: Use SimpleArray, json is fundamentally different and requires a migration.
johanib
force-pushed
the
feature/update_dbal_4_1799_3
branch
from
271256a to
cb43e68
Compare
johanib
force-pushed
the
feature/update_dbal_4_1799_3
branch
from
cb43e68 to
fdc995d
Compare
johanib
force-pushed
the
feature/update_dbal_4_1799_3
branch
from
fdc995d to
84b50bb
Compare
Prior to this change, the DoctrineConnectionCheck from the monitor bundle was used instead of the check from EB. This became apparent as the doctrine/dbal broke the DoctrineConnectionHealthCheck from the Monitor bundle. See #1902
github-project-automation
bot
moved this from In Progress
to Backlog
in PHP development
3 participants
Needed for SF 6.4