Skip to content

Feature: Doctrine Provider #253

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feature: Doctrine Provider #253

wants to merge 2 commits into from

Conversation

joelwurtz
Copy link
Member

@joelwurtz joelwurtz commented Apr 2, 2025

This allow automatically provide an entity from database instead of creating a new one, which will update values instead of generating a new entity

This is rather a first simple implementation, but it should work for the 90% use case

@joelwurtz joelwurtz marked this pull request as ready for review August 4, 2025 09:23
@joelwurtz joelwurtz requested a review from Korbeil August 4, 2025 09:32
Copy link
Member

@Korbeil Korbeil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's nice to base mapping on Doctrine metadata factory.
I would like to see something like an attribute / context thing to say "this is a Doctrine entity but I don't want to use entity manager just map it normally".
Also I think it would be nice to have extension points, at the moment it is very rigid and we can't do much to extends it (mostly Doctrine provider in my opinion).

$providers = iterator_to_array($providers);

if (null !== $entityManager) {
$providers[] = new DoctrineProvider($entityManager);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can't surcharge this provider and its static ? is there any way we could provide it with configuration so we can surcharge it ?

Comment on lines +25 to +27
/** @var class-string<object>|'array' $source */
public string $source,
/** @var class-string<object>|'array' $target */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't it makes a duplicate of the phpdoc we have just above ?

@@ -20,5 +20,5 @@ interface ProviderInterface
*
* @return object|array<mixed>|null the value to provide
*/
public function provide(string $targetType, mixed $source, array $context): object|array|null;
public function provide(string $targetType, mixed $source, array $context, /* mixed $id */): object|array|null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use context for this identifier ? Instead of modifying all the providers for only one provider ?

Copy link
Collaborator

@nikophil nikophil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really nice feature 👍

WDYT of also supporting doctrine Mongo ODM?

since you're mainly relying on ClassMetadataFactory which is an abstraction from doctrine/persistence it should not require too much effort

I would like to see something like an attribute / context thing to say "this is a Doctrine entity but I don't want to use entity manager just map it normally".

💯

"doctrine/collections": "^2.2",
"doctrine/inflector": "^2.0",
"doctrine/orm": "^3.3",
Copy link
Collaborator

@nikophil nikophil Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this feature won't be tested with orm 2 (there are still a lot of them in the wild) ?

@@ -176,6 +179,12 @@ public static function create(
$classDiscriminatorFromClassMetadata = new ClassDiscriminatorFromClassMetadata($classMetadataFactory);
}

$providers = iterator_to_array($providers);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so all those providers will be instantiated here?

@@ -43,6 +43,7 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->booleanNode('serializer_attributes')->defaultValue(interface_exists(SerializerInterface::class))->end()
->booleanNode('api_platform')->defaultFalse()->end()
->booleanNode('doctrine')->defaultFalse()->end()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be autodiscovered? (ie: doctrine bundle is enabled)

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

Successfully merging this pull request may close these issues.

3 participants