Simplify TypeGenerator dependencies

[murtukov]

Q	A
RFC?	yes
Version/Branch	0.14

The TypeGeneratorclass has a lot of dependencies and the most of them are Symfony parameters. The current service declaration looks like this:

GraphQLBundle/src/Resources/config/services.yaml

Lines 63 to 72 in f0af747

	Overblog\GraphQLBundle\Generator\TypeGenerator:
	arguments:
	- "%overblog_graphql.class_namespace%"
	- "%overblog_graphql.cache_dir%"
	- "%overblog_graphql_types.config%"
	- '@Overblog\GraphQLBundle\Generator\TypeBuilder'
	- '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
	- "%overblog_graphql.use_classloader_listener%"
	- "%kernel.cache_dir%"
	- "%overblog_graphql.cache_dir_permissions%"

and the class constructor with properties looks like this:

GraphQLBundle/src/Generator/TypeGenerator.php

Lines 30 to 64 in f0af747

	private static bool $classMapLoaded = false;
	private ?string $cacheDir;
	protected int $cacheDirMask;
	private array $configs;
	private bool $useClassMap;
	private ?string $baseCacheDir;
	private string $classNamespace;
	private TypeBuilder $typeBuilder;
	private EventDispatcherInterface $eventDispatcher;
	public function __construct(
	string $classNamespace,
	?string $cacheDir,
	array $configs,
	TypeBuilder $typeBuilder,
	EventDispatcherInterface $eventDispatcher,
	bool $useClassMap = true,
	?string $baseCacheDir = null,
	?int $cacheDirMask = null
	) {
	$this->cacheDir = $cacheDir;
	$this->configs = $configs;
	$this->useClassMap = $useClassMap;
	$this->baseCacheDir = $baseCacheDir;
	$this->typeBuilder = $typeBuilder;
	$this->eventDispatcher = $eventDispatcher;
	$this->classNamespace = $classNamespace;
	if (null === $cacheDirMask) {
	// Apply permission 0777 for default cache dir otherwise apply 0775.
	$cacheDirMask = null === $cacheDir ? 0777 : 0775;
	}
	$this->cacheDirMask = $cacheDirMask;
	}

in my opinion it looks messy. What I would like to do is to pass all the parameter dependencies together, either as an array or an object and maybe rename some of them to make them more descriptive. I considered 3 different ways to do that, but I will describe here only 1 of them, which I think is the best way:

Change the service declaration to pass all the parameters as an array:

Overblog\GraphQLBundle\Generator\TypeGenerator:
    arguments:
        - '@Overblog\GraphQLBundle\Generator\TypeBuilder'
        - '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
        - classNamespace: "%overblog_graphql.class_namespace%"
          cacheDir:       "%overblog_graphql.cache_dir%"
          config:         "%overblog_graphql_types.config%"
          useClassMap:    "%overblog_graphql.use_classloader_listener%"
          baseCacheDir:   "%kernel.cache_dir%"
          cacheDirMask:   "%overblog_graphql.cache_dir_permissions%"

To make the service declaration more readable we could use explicit arguments:

Overblog\GraphQLBundle\Generator\TypeGenerator:
    arguments:
        $typeBuilder:     '@Overblog\GraphQLBundle\Generator\TypeBuilder'
        $eventDispatcher: '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
        $options:
            namespace:    '%overblog_graphql.class_namespace%'
            config:       "%overblog_graphql_types.config%"
            casheDir:     "%overblog_graphql.cache_dir%"
            useClassMap:  "%overblog_graphql.use_classloader_listener%"
            cacheDir:     "%kernel.cache_dir%"
            cacheDirMask: "%overblog_graphql.cache_dir_permissions%"

class constructor:

public function __construct(TypeBuilder $typeBuilder, EventDispatcherInterface $eventDispatcher, array $options) 
{
    $this->typeBuilder = $typeBuilder;
    $this->eventDispatcher = $eventDispatcher;
    $this->options = new GeneratorOptions($options);
}

As you can see, the class has now only 3 dependencies. Parameters are combined into the $options array and passed into GeneratorOptions class which is just a DTO to avoid working with arrays. It destructures the array and creates a nice object with autocompletion. Also since properties are typed, there is no need to create getters/setters:

class GeneratorOptions
{
    public ?string $cacheDir;
    public int $cacheDirMask;
    public array $configs;
    public bool $useClassMap;
    public ?string $baseCacheDir;
    public string $classNamespace;

    public function __construct($options)
    {
        $this->classNamespace = $options['classNamespace'];
        $this->cacheDir       = $options['cacheDir'];
        $this->configs        = $options['config'];
        $this->useClassMap    = $options['useClassMap'] ?? true;
        $this->baseCacheDir   = $options['baseCacheDir'] ?? null;
        $this->cacheDirMask   = $options['cacheDirMask'] ?? (
            // Apply permission 0777 for default cache dir otherwise apply 0775.
            null === $options['cacheDir'] ? 0777 : 0775
        );
    }
}

All this would greatly reduce the size of TypeGenerator class to a couple of methods and if you need to access the options, you just call the options object and work with it:

$options = $typeGenerator->options;

$options->cacheDir;
$options->baseCacheDir;
// etc...

As another option the GeneratorOptions object could be passed to TypeGenerator and not instantiated in it's constructor.

[mcg-web]

LGTM, but I would be easier if we inject directly TypeGeneratorOptions:

public function __construct(TypeBuilder $typeBuilder, EventDispatcherInterface $eventDispatcher, TypeGeneratorOptions $options) 
{
    $this->typeBuilder = $typeBuilder;
    $this->eventDispatcher = $eventDispatcher;
    $this->options = $options;
}

Also making it a service with readonly properties after instanciation .

Overblog\GraphQLBundle\Generator\TypeGenerator:
    arguments:
        $typeBuilder:     '@Overblog\GraphQLBundle\Generator\TypeBuilder'
        $eventDispatcher: '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
        $options: '@Overblog\GraphQLBundle\Generator\TypeGeneratorOptions'

Overblog\GraphQLBundle\Generator\TypeGeneratorOptions:
    arguments:
            $namespace:    '%overblog_graphql.class_namespace%'
            $config:       "%overblog_graphql_types.config%"
            $casheDir:     "%overblog_graphql.cache_dir%"
            $useClassMap:  "%overblog_graphql.use_classloader_listener%"
            $cacheDir:     "%kernel.cache_dir%"
            $cacheDirMask: "%overblog_graphql.cache_dir_permissions%"

class TypeGeneratorOptions
{
    private ?string $cacheDir;
    private int $cacheDirMask;
    private array $configs;
    private bool $useClassMap;
    private ?string $baseCacheDir;
    private string $classNamespace;

    public function __construct(
       string $classNamespace, 
       ?string $cacheDir, 
       array $configs,
       bool $useClassMap = true, 
       ?string $baseCacheDir = null, 
       ?int $cacheDirMask = null 
    )
    {
        $this->classNamespace = $classNamespace;
        $this->cacheDir       = $cacheDir;
        $this->configs        =  $configs;
        $this->useClassMap    = $useClassMap ?? true;
        $this->baseCacheDir   = $baseCacheDir ?? null;
        $this->cacheDirMask   = $cacheDirMask ?? (
            // Apply permission 0777 for default cache dir otherwise apply 0775.
            null === $cacheDir ? 0777 : 0775
        );
    }

    public function __get(string $name)
    {
         return $this->$name;
    }
}

[murtukov]

@mcg-web agree, except the read-only properties, because:

1. It disables the autocompletion on private properties, which is the reason why we make it an object.
2. Some properties must be writable, because they are changed in some places.

[mcg-web]

Ok @murtukov, so no need to use a construct in this case. We can use an anonymous service to prevent options change after generation has already be run:

class TypeGeneratorOptions
{
    public ?string $cacheDir;
    public int $cacheDirMask;
    public array $configs;
    public bool $useClassMap;
    public ?string $baseCacheDir;
    public string $classNamespace;
}

Overblog\GraphQLBundle\Generator\TypeGenerator:
    arguments:
        $typeBuilder:     '@Overblog\GraphQLBundle\Generator\TypeBuilder'
        $eventDispatcher: '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
        $options:  !service
                class: Overblog\GraphQLBundle\Generator\TypeGeneratorOptions:
                properties:
                    namespace:    '%overblog_graphql.class_namespace%'
                    config:       "%overblog_graphql_types.config%"
                    casheDir:     "%overblog_graphql.cache_dir%"
                    useClassMap:  "%overblog_graphql.use_classloader_listener%"
                    cacheDir:     "%kernel.cache_dir%"
                    cacheDirMask: "%overblog_graphql.cache_dir_permissions%"

this part can be made on runtime:

$cacheDirMask ?? (
    // Apply permission 0777 for default cache dir otherwise apply 0775.
    null === $cacheDir ? 0777 : 0775
);