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

Open
wants to merge 1 commit into
base: 2.12.x
Choose a base branch
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.

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
2 participants