Skip to content

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Feb 17, 2025

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT
  • Attaching the Persist & Delete processors automatically via the ManagerRegistry
  • Deprecating driver configuration in ResourceMetadata
  • Deprecating driver configuration in Bundle configuration (should be done when all the new routing system will be available in Sylius E-commerce)

The Persist & Remove processors already use the Repository managed by Doctrine.
But the system was using a driver detection using the config (by default, a resource is configured with 'doctrine/orm' as driver).

At the end, we'll be able to use the Resource without any driver configuration. You can look at the current DoctrineOrmDriver to see how much the complexity is.

With this new system, if you use a Repository managed by Doctrine, it will be used automatically, otherwise, you are able to use custom providers which are more powerful (retrieve sth from anywhere you want).

Moreover, the driver system has some issues with the repository (see #981)
So it's hard to maintain & trust the Sylius Repository configuration system.
Indeed, the repository to be used by Doctrine is configured on Runtime.

We have this System in ApiPlatform
https://github.com/api-platform/core/blob/main/src/Doctrine/Orm/Metadata/Resource/DoctrineOrmResourceCollectionMetadataFactory.php

More explanation about driver false

Setting driver to false already exists, it disables the driver system. It was introduced to detach a resource from Doctrine when the resource was not an entity.
In this case, there's no built-in factory & repository.

About repository DI

with current implementation, RepositoryInterface was generic and that's why it was introduced a binding via the repository name which is not well-known and not documented.

class Test 
{
    public function __construct(
        private RepositoryInterface $bookRepository, // It injects 'app.repository.book'
    ) {}
}

In Symfony, by default, when we use the Entity maker command, we also have a service entity repository which is automatically created.

namespace App\Repository;

use App\Entity\Book;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;

class BookRepository extends ServiceEntityRepository
{
    public function __construct(ManagerRegistry $registry)
    {
        parent::__construct($registry, Book::class);
    }
}

Cause this is a service, it could be injected directly or with an interface. This is the normal way to inject services in Symfony.

use App\Repository\BookRepository;
use App\Repository\BookRepositoryInterface;

class Test 
{
    public function __construct(
        private BookRepository $bookRepository,
        // Or private BookRepositoryInterface $bookRepository,
    ) {}
}

In Sylius E-commerce, we still need to create this "sylius.repository.order" but we'll need to declare these services manually instead as we do for every services.

If you have a generic EntityRepository, it's still possible to configure a binding with name via Symfony DI configuration.
It could be automatic, this "automation" already exists in this package and could be moved to Sylius E-commerce where the repository will be created with an app.repository.book id.

# config/services.yaml
services:
    _defaults:
        bind: 
            EntityRepository $bookRepository: '@app.repository.book'

RepositoryInterface is already optional

Implementing this interface is already optional since this new resource metadata implementation.
The Persist and Remove processors retrieve the repository managed by Doctrine.
So this PR just use the same solution with providers.

@loic425 loic425 changed the title Use Repository managed by Doctrine Use Repository managed by Doctrine automatically Feb 17, 2025
@loic425 loic425 force-pushed the feature/use-repository-in-manager-registry branch 3 times, most recently from 7d7a59e to bf76da2 Compare February 17, 2025 14:48
@loic425 loic425 marked this pull request as draft February 17, 2025 14:57
@loic425 loic425 force-pushed the feature/use-repository-in-manager-registry branch from bf76da2 to 5c399aa Compare February 17, 2025 15:01
@loic425 loic425 marked this pull request as ready for review February 17, 2025 15:03
$operation = $operation->withResource($resource);

if (null === $operation->getRepository()) {
$operation = $operation->withRepository($resourceConfiguration->getServiceId('repository'));
Copy link
Member Author

Choose a reason for hiding this comment

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

We do not use this repository by default anymore.

}

if (null === $operation->getProvider()) {
$operation = $operation->withProvider(Provider::class);
Copy link
Member Author

@loic425 loic425 Feb 17, 2025

Choose a reason for hiding this comment

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

This Provider was kind of Doctrine related, we use another improved one which is configured if the Resource is managed by Doctrine only.

$this->assertInstanceOf(ProviderResourceMetadataCollectionFactory::class, $this->factory);
}

public function testItCreatesResourceMetadataWithDefaultProviderOnHttpOperations(): void
Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed this default provider which was a lot related to Doctrine.

App\Subscription\:
resource: '../src/Subscription'

App\Subscription\Factory\SubscriptionFactory:
Copy link
Member Author

Choose a reason for hiding this comment

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

We can still use a factory service here but the app.factory.subscription doesn't exist if we do not configure any driver.
So It's more complicated like this, cause we need to tag this factory to be used in the repository locator.

final class Provider implements ProviderInterface
{
public function __construct(
private ContainerInterface $locator,
Copy link
Member Author

@loic425 loic425 Feb 17, 2025

Choose a reason for hiding this comment

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

Hopefully, this provider is experimental, so we could move it and improve it.

@loic425 loic425 force-pushed the feature/use-repository-in-manager-registry branch 4 times, most recently from 193218c to 1ac8171 Compare February 19, 2025 08:57
@loic425 loic425 force-pushed the feature/use-repository-in-manager-registry branch from 1ac8171 to d8655a6 Compare February 19, 2025 09:16
@loic425 loic425 force-pushed the feature/use-repository-in-manager-registry branch from fc5fb9b to fb88f77 Compare February 19, 2025 15:25
@loic425 loic425 force-pushed the feature/use-repository-in-manager-registry branch from fb88f77 to 960c0f7 Compare February 19, 2025 15:27
@loic425 loic425 self-assigned this Feb 21, 2025
<services>
<defaults public="true" />

<service id="sylius.doctrine_orm.metadata.resource.metadata_collection_factory"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<service id="sylius.doctrine_orm.metadata.resource.metadata_collection_factory"
<service id="sylius.resource_metadata_collection.factory.doctrine.orm"

Would make it easier to find since it'd be in line with the decorated service

<argument type="service" id=".inner" />
</service>

<service id="sylius.state_provider.doctrine.orm.state.provider" class="Sylius\Resource\Doctrine\ORM\State\Provider">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<service id="sylius.state_provider.doctrine.orm.state.provider" class="Sylius\Resource\Doctrine\ORM\State\Provider">
<service id="sylius.state_provider.doctrine.orm" class="Sylius\Resource\Doctrine\ORM\State\Provider">

Seems a bit superfluous

Comment on lines +40 to +41
/** @var ResourceMetadata $resource */
foreach ($resourceCollectionMetadata->getIterator() as $i => $resource) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var ResourceMetadata $resource */
foreach ($resourceCollectionMetadata->getIterator() as $i => $resource) {
foreach ($resourceCollectionMetadata->getIterator() as $i => $resource) {

Setting phpstan params (TKey, TValue) on ResourceMetadataCollection would ease type-hinting

return null;
}

return 'sylius.state_provider.doctrine.orm.state.provider';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put it in a const, or pass through the constructor as a parameter?

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