phoenix icon indicating copy to clipboard operation
phoenix copied to clipboard

Add typed_fields option to ORM configuration to configure TypedFieldMapper

Open tijnema opened this issue 1 year ago • 3 comments

This fixes #1696

Configuration syntax is based on the suggestion by @nicolas-grekas here

Love to hear your thoughts, although I'm not an expert in this area.

tijnema avatar Mar 19 '24 23:03 tijnema

This is a good starting point, but I want to highlight that this doesn't support custom type field mappers. I'm afraid if we did this, in few months someone will complain they can't define custom mapper through configuration. Any suggestions to avoid that? Other than that, this PR is completely missing tests of course. And DefaultTypeFieldMapper service should be defined in orm.xml file. DoctrineExtension will then only reference to it.

ostrolucky avatar Mar 21 '24 21:03 ostrolucky

I see your point. This would be the easiest for users to configure, but not the most flexible. Another solution I see is allowing one to simply set a TypedFieldMapper service.

dbal:
    orm:
        typed_field_mapper: '@Doctrine\ORM\Mapping\DefaultTypedFieldMapper'

Which then needs to be configured in services.yaml

If that's the preferred way, I can work on that (including tests)

tijnema avatar Mar 22 '24 08:03 tijnema

Let's go with this: For now we don't need support for something else than DefaultTypedFieldMapper. In future if there is a need for custom field mapper, we will add another key (like the one you proposed) and disallow combining it with typed_fields keys in this PR. So this PR is quite feature complete (at least for now), but please add tests.

ostrolucky avatar Apr 14 '24 19:04 ostrolucky

Hi, this issue is open for over a year and is blocking the rest of the 2.14.0 release.

Is there a way we could move forwards with it, or maybe move it to 2.15.0 (or later) milestone?

kochen avatar Mar 18 '25 09:03 kochen

As can be seen by my comment and PR label, tests are needed to be added

ostrolucky avatar Mar 18 '25 09:03 ostrolucky

As can be seen by my comment and PR label, tests are needed to be added

@tijnema any chance you could work on that soon?

@ostrolucky could we move this issue to 2.15.0 for now and release 2.14.0 sooner?

kochen avatar Mar 18 '25 09:03 kochen

For my current project we don't really need this anymore. Not sure if this PR is still relevant actually. Isn't everything needed implemented with #1802 ?

tijnema avatar Mar 18 '25 12:03 tijnema

Indeed, so I guess this is a duplicate!

ostrolucky avatar Mar 18 '25 12:03 ostrolucky