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

Drivers can now be configured #735

Open
wants to merge 4 commits into
base: 1.11
Choose a base branch
from

Conversation

Prometee
Copy link
Contributor

@Prometee Prometee commented Jul 7, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

This PR allows to configure any drivers you want or even change the default one.

Here is the previous default configuration :

sylius_resource:
    drivers:
        - 'doctrine/orm'

Now you can setup a little bit more info :

sylius_resource:
    drivers:
        doctrine/orm:
            class: 'Sylius\Bundle\ResourceBundle\DependencyInjection\Driver\Doctrine\DoctrineORMDriver'

To keep backward compatibility, you still can set the config with a simple string array when you are using one the 3 available drivers : doctrine/*

Right now this PR is using only the class config member, but we can have other things added here in the futur.

Custom driver example

You are creating your own driver and wanted to register it in the list of available drivers, here is how to proceed :

# config/packages/sylius_resource.yaml
sylius_resource:
    drivers:
        0: doctrine/orm # Add default value
        custom_driver:
            class: 'MyBundleOrApp\SyliusResource\Driver\MyCustomDriver'
<?php

declare(strict_types=1);

namespace MyBundleOrApp\SyliusResource\Driver;

use Sylius\Bundle\ResourceBundle\DependencyInjection\Driver\DriverInterface;
use Sylius\Component\Resource\Metadata\MetadataInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

final class MyCustomDriver implement DriverInterface {

    public function getType(): string
    {
        return 'custom_driver';
    }

    public function load(ContainerBuilder $container, MetadataInterface $metadata): void
    {
        ...
    }
}

@Prometee Prometee requested a review from a team as a code owner July 7, 2023 11:43
@Prometee Prometee force-pushed the configurable-drivers branch 3 times, most recently from 1aed7e9 to 8c2a866 Compare July 7, 2023 12:21
@Prometee Prometee force-pushed the configurable-drivers branch from 8c2a866 to 6eff4c4 Compare July 7, 2023 12:28
src/Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
src/Bundle/DependencyInjection/Driver/DriverProvider.php Outdated Show resolved Hide resolved
/** @var class-string|null $class */
$class = self::$drivers[$type]['class'] ?? null;
if (null !== $class) {
$driver = new $class();
Copy link
Member

Choose a reason for hiding this comment

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

Hum, That'd be better to use service instead of instantiating class, we can't have dependency injection in the drivers with your implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't use a service because all the parameters created by the SyliusResourceBundle will have to be created during configuration. I don't know yet why this choice have been done, maybe there is a good reason.

Copy link
Member

Choose a reason for hiding this comment

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

@lchrusciel WDYT?

use Sylius\Component\Resource\Metadata\MetadataInterface;
use Webmozart\Assert\Assert;

final class DriverProvider
{
/** @var DriverInterface[] */
/** @var array<string, array<string, string>> */
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too familiar with this construction, but will this work?

--- @var array<string, array<string, string>>
+++ @var array<string, array<string, class-string<DriverInterface>>>

Will it remove need to specify

       /** @var class-string|null $class */
        $class = self::$drivers[$type]['class'] ?? null;

and

           /** @var DriverInterface $driver */
            $driver = new $class();

later in the file?

Copy link
Contributor Author

@Prometee Prometee Sep 1, 2023

Choose a reason for hiding this comment

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

Yes It works in this case, but the attribute php doc is here to define the generic type of a driver configuration. We should perhaps build a DriverMetadata class which own this configuration to better type it (I have to check if @loic425 already did this)

return self::$drivers[$type];
}
/** @var class-string|null $class */
$class = self::$drivers[$type]['class'] ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

This line is somewhat misleading as $class is made nullable, while only $drivers[$type] is nullable and $drivers[$type]['class'] should be asserted

if (isset(self::$drivers[$type])) {
    Assert::notNull(self::$drivers[$type]['class'], sprintf('Class must be present for driver "%s" configuration', $type));
    $class = self::$drivers[$type]['class'];
    
    return new $class();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO this kind of assertion must be done into a DriverMetadata class like I suggest earlier.

@Zales0123 Zales0123 added the Feature New feature proposals. label Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants