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

Add typed_fields option to ORM configuration to configure TypedFieldMapper #1779

Closed
wants to merge 1 commit into from

Conversation

tijnema
Copy link

@tijnema tijnema commented Mar 19, 2024

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.

@ostrolucky
Copy link
Member

ostrolucky commented Mar 21, 2024

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.

@tijnema
Copy link
Author

tijnema commented Mar 22, 2024

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)

@ostrolucky
Copy link
Member

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 ostrolucky changed the base branch from 2.12.x to 2.13.x September 1, 2024 09:48
@ostrolucky ostrolucky changed the base branch from 2.13.x to 2.14.x September 1, 2024 09:52
@ostrolucky ostrolucky added this to the 2.14.0 milestone Sep 1, 2024
@kochen
Copy link
Contributor

kochen commented Mar 18, 2025

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?

@ostrolucky
Copy link
Member

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

@kochen
Copy link
Contributor

kochen commented Mar 18, 2025

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?

@ostrolucky ostrolucky removed this from the 2.14.0 milestone Mar 18, 2025
@tijnema
Copy link
Author

tijnema commented Mar 18, 2025

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 ?

@ostrolucky
Copy link
Member

Indeed, so I guess this is a duplicate!

@ostrolucky ostrolucky closed this Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Add support for TypeFieldMapper
3 participants