Implement ConfigBuilder

[gennadigennadigennadi]

First impression is that the current Configuration is a bit off, at least for the auto generated configbuilders

[patrickkusebauch]

I really like the start. I cannot comment in the generated classes, but commenting on the actual config usage. It is clear to me that moving to PHP Config would require rethinking how the structure needs to look like

[patrickkusebauch]

Is there a possibility of "working back"? What I mean is: Given a sample configuration usage, can we create a ConfigBuilder that would enable such usage?

[gennadigennadigennadi]

I think there is now enough code to discuss the current implementation/design of DeptracConfig with its fluent setter functions. @dbrumann what to you think?
What to you guys (@patrickkusebauch , @dbrumann) think about the baseline configuration part?
I don't know how we want to configure services via php nicely. Currently is only possible if we set ContainerConfigurator as a second parameter and use it directly (it's the symfony native one, what I don't like).

There are few things that still need to be done. E.g. Formatter, tests and optimise some of the implementation details of the ConfigClasses.

[dbrumann]

Overall this looks really promising. The php config is still rather verbose, but that's just how it is, I guess. The config objects will give us a nice way of simplifying things in the future, including the oddities around regex escaping, so 👍 from me

[patrickkusebauch]

All the config files should be in the Contract namespace, not Supportive.

Otherwise, I am really happy with the change. Great job!

[gennadigennadigennadi]

This is would be another possible config. It uses static factories methods instead of the symfony configurator style.
Which one do you guys prefer @patrickkusebauch, @dbrumann ?

<?php

use Internal\Qossmic\Deptrac\IgnoreDependenciesOnContract;
use Internal\Qossmic\Deptrac\IgnoreDependenciesOnShouldNotHappenException;
use Qossmic\Deptrac\Contract\Analyser\ProcessEvent;
use Qossmic\Deptrac\Contract\Config\Collector\BoolConfig;
use Qossmic\Deptrac\Contract\Config\Collector\ClassNameConfig;
use Qossmic\Deptrac\Contract\Config\Collector\DirectoryConfig;
use Qossmic\Deptrac\Contract\Config\DeptracConfig;
use Qossmic\Deptrac\Contract\Config\EmitterType;
use Qossmic\Deptrac\Contract\Config\Layer;
use Qossmic\Deptrac\Contract\Config\RulesetConfig;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (DeptracConfig $config, ContainerConfigurator $containerConfigurator): void {
    $services = $containerConfigurator->services();
    $services->set(IgnoreDependenciesOnContract::class)->tag('kernel.event_listener', ['event' => ProcessEvent::class]);
    $services->set(IgnoreDependenciesOnShouldNotHappenException::class)->tag(
        'kernel.event_listener',
        ['event' => ProcessEvent::class]
    );

    $config
        ->paths('src')
        ->baseline('deptrac.baseline.yaml')
        ->analyser(
            EmitterType::CLASS_TOKEN,
            EmitterType::CLASS_SUPERGLOBAL_TOKEN,
            EmitterType::FILE_TOKEN,
            EmitterType::FUNCTION_TOKEN,
            EmitterType::FUNCTION_SUPERGLOBAL_TOKEN,
            EmitterType::FUNCTION_CALL,
        )
        ->layers(
            $analyser = Layer::withName('Analyser')->collectors(
                DirectoryConfig::public('src/Core/Analyser/.*')
            ),
            $ast = Layer::withName('Ast')->collectors(
                DirectoryConfig::public('src/Core/Ast/.*'),
                ClassNameConfig::private('^PHPStan\\\\PhpDocParser\\\\.*'),
                ClassNameConfig::private('^PhpParser\\\\.*'),
                ClassNameConfig::private('^phpDocumentor\\\\Reflection\\\\.*')
            ),
            $console = Layer::withName('Console')->collectors(
                DirectoryConfig::public('src/Supportive/Console/.*')
            ),
            $dependency = Layer::withName('Dependency')->collectors(
                DirectoryConfig::public('src/Core/Dependency/.*')
            ),
            $dependencyInjection = Layer::withName('DependencyInjection')->collectors(
                DirectoryConfig::public('src/Supportive/DependencyInjection/.*')
            ),
            $contract = Layer::withName('Contract')->collectors(
                DirectoryConfig::public('src/Contract/.*')
            ),
            $inputCollector = Layer::withName('InputCollector')->collectors(
                DirectoryConfig::public('src/Core/InputCollector/.*')
            ),
            $layer = Layer::withName('Layer')->collectors(
                DirectoryConfig::public('src/Core/Layer/.*')
            ),
            $outputFormatter = Layer::withName('OutputFormatter')->collectors(
                DirectoryConfig::public('src/Supportive/OutputFormatter/.*'),
                ClassNameConfig::private('^phpDocumentor\\\\GraphViz\\\\.*'),
            ),
            $file = Layer::withName('File')->collectors(
                DirectoryConfig::public('src/Supportive/File/.*')
            ),
            $supportive = Layer::withName('Supportive')->collectors(
                BoolConfig::public()
                    ->withMustNot(DirectoryConfig::public('src/Supportive/.*/.*'))
                    ->withMust(DirectoryConfig::public('src/Supportive/.*'))
            ),
        )
        ->rulesets(
            RulesetConfig::layer($layer)->accesses($ast),
            RulesetConfig::layer($console)->accesses($analyser, $outputFormatter, $dependencyInjection, $file),
            RulesetConfig::layer($dependency)->accesses($ast),
            RulesetConfig::layer($analyser)->accesses($layer, $dependency, $ast),
            RulesetConfig::layer($outputFormatter)->accesses($console, $dependencyInjection),
            RulesetConfig::layer($ast)->accesses($file, $inputCollector),
            RulesetConfig::layer($inputCollector)->accesses($file),
            RulesetConfig::layer($supportive),
            RulesetConfig::layer($contract),
        );
};

and for comparision the current symfony style for the deptrac.config.php

use Internal\Qossmic\Deptrac\IgnoreDependenciesOnContract;
use Internal\Qossmic\Deptrac\IgnoreDependenciesOnShouldNotHappenException;
use Qossmic\Deptrac\Contract\Analyser\ProcessEvent;
use Qossmic\Deptrac\Contract\Config\DeptracConfig;
use Qossmic\Deptrac\Contract\Config\CollectorType;
use Qossmic\Deptrac\Contract\Config\EmitterType;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (DeptracConfig $config, ContainerConfigurator $containerConfigurator): void {
    $services = $containerConfigurator->services();
    $services->set(IgnoreDependenciesOnContract::class)->tag('kernel.event_listener', ['event' => ProcessEvent::class]);
    $services->set(IgnoreDependenciesOnShouldNotHappenException::class)->tag('kernel.event_listener', ['event' => ProcessEvent::class]);

    $config->paths('src');
    //$config->baseline('deptrac.baseline.yaml');

    // analyser
    $config->analyser(
        EmitterType::CLASS_TOKEN,
        EmitterType::CLASS_SUPERGLOBAL_TOKEN,
        EmitterType::FILE_TOKEN,
        EmitterType::FUNCTION_TOKEN,
        EmitterType::FUNCTION_SUPERGLOBAL_TOKEN,
        EmitterType::FUNCTION_CALL,
    );

    // layer
    $analyser = $config->layer('Analyser');
    $analyser->collector(CollectorType::TYPE_DIRECTORY)->value('src/Core/Analyser/.*');

    $ast = $config->layer('Ast');
    $ast->collector(CollectorType::TYPE_DIRECTORY)->value('src/Core/Ast/.*');
    $ast->collector(CollectorType::TYPE_CLASS_NAME)->value('^PHPStan\\\\PhpDocParser\\\\.*')->private();
    $ast->collector(CollectorType::TYPE_CLASS_NAME)->value('^PhpParser\\\\.*')->private();
    $ast->collector(CollectorType::TYPE_CLASS_NAME)->value('^phpDocumentor\\\\Reflection\\\\.*')->private();

    $console = $config->layer('Console');
    $console->collector(CollectorType::TYPE_DIRECTORY)->value('src/Supportive/Console/.*');

    $dependency = $config->layer('Dependency');
    $dependency->collector(CollectorType::TYPE_DIRECTORY)->value('src/Core/Dependency/.*');

    $dependencyInjection = $config->layer('DependencyInjection');
    $dependencyInjection->collector(CollectorType::TYPE_DIRECTORY)->value('src/Supportive/DependencyInjection/.*');

    $contract = $config->layer('Contract');
    $contract->collector(CollectorType::TYPE_DIRECTORY)->value('src/Contract/.*');

    $inputCollector = $config->layer('InputCollector');
    $inputCollector->collector(CollectorType::TYPE_DIRECTORY)->value('src/Core/InputCollector/.*');

    $layer = $config->layer('Layer');
    $layer->collector(CollectorType::TYPE_DIRECTORY)->value('src/Core/Layer/.*');

    $outputFormatter = $config->layer('OutputFormatter');
    $outputFormatter->collector(CollectorType::TYPE_DIRECTORY)->value('src/Supportive/OutputFormatter/.*');
    $outputFormatter->collector(CollectorType::TYPE_CLASS_NAME)->value('^phpDocumentor\\\\GraphViz\\\\.*')->private();

    $file = $config->layer('File');
    $file->collector(CollectorType::TYPE_DIRECTORY)->value('src/Supportive/File/.*');

    $supportive = $config->layer('Supportive');
    $supportiveCollector = $supportive->collector(CollectorType::TYPE_BOOL);
    $supportiveCollector->mustNot(CollectorType::TYPE_DIRECTORY)->value('src/Supportive/.*/.*');
    $supportiveCollector->must(CollectorType::TYPE_DIRECTORY)->value('src/Supportive/.*');

    // ruleset
    $config->ruleset($layer)->accessesLayer($ast);
    $config->ruleset($console)->accessesLayer($analyser, $outputFormatter, $dependencyInjection, $file);
    $config->ruleset($dependency)->accessesLayer($ast);
    $config->ruleset($analyser)->accessesLayer($layer, $dependency, $ast);
    $config->ruleset($outputFormatter)->accessesLayer($console, $dependencyInjection);
    $config->ruleset($ast)->accessesLayer($file, $inputCollector);
    $config->ruleset($inputCollector)->accessesLayer($file);
    

[patrickkusebauch]

I like the first option. It is way more readable to me. 2 ideas I have just from looking at it:

• All calls to DeptracConfig like layers and rulesets need to be additive. That way you can compose from multiple files.
• ClassNameConfig::private('^phpDocumentor\\\\GraphViz\\\\.*') could become ClassNameConfig::private('^phpDocumentor\GraphViz\.*') or at least ClassNameConfig::private('^phpDocumentor\\GraphViz\\.*') which would make it more user friendly.

[gennadigennadigennadi]

• All calls to DeptracConfig like layers and rulesets need to be additive. That way you can compose from multiple
  files.

The functions layers and rulesets just add Objects to an array, so there is no problem if you like to call them multiple times.

• ClassNameConfig::private('^phpDocumentor\\\\GraphViz\\\\.*') could become ClassNameConfig::private('^phpDocumentor\GraphViz\.*') or at least ClassNameConfig::private('^phpDocumentor\\GraphViz\\.*') which would make it more user friendly.

To solve this problem I copied the escaping logic from the Symfony yaml component into the function regex(). If we would bake the escaping logic into the CollectorConfig I think more advance php developer would ran into double escaped regexes.

[gennadigennadigennadi]

@dbrumann, @patrickkusebauch did you guys also forgot, that we won't get autocompletion from the classes inside our .phar? PHPStorm does some magic and looks inside our deptrac.phar but not all IDE do. For me that's a deal breaker.

Maybe we should work on #916 first ;-), our bundle the .phar with the contract-classes.

[dbrumann]

I really like the current state. The PHP config is easy to read and thanks to the builders we can have some nice DX features like auto-escaping the regex. 👍

Gennadi and I discussed this PR on Friday. We decided that this should be an optional, experimental feature in the current major version. This will allow us to try this out first, adjust things based on early feedback, and then switch from YAML to PHP as default with one of the upcoming major versions.

We will keep YAML around for anyone who doesn't want to change their configs for as long as both config formats can stay interchangeable. However, we will not maintain docs for both config formats.

[patrickkusebauch]

@gennadigennadigennadi is this one ready for review?

[gennadigennadigennadi]

Yes, please have a look.

I'll have to rebase, but otherwise it's ready for review.

But even if we merge it, it's not production ready. As mentioned by @dbrumann it this experimental.
I'll have a look next week if we can also add a stub, so we can get autocomplete for all IDE's.

[patrickkusebauch]

Some questions about what is being done, but otherwise very happy about it.

[patrickkusebauch]

Can we have a key->value array instead of 2 lists? It would make it IMHO more readable and less prone to error.

[gennadigennadigennadi]

This data is borrowed from symfony and I don't really know what all codes do. So wouldn't change it.

[patrickkusebauch]

You are just replacing the found instances of ESCAPEES with ESCAPED at the same position in the array. If it was just one key->value array you would just change it to str_replace(array_keys(self::ARRAY), array_values(self::ARRAY), $regex). That would be more readable to me. Not a blocker though.

[dbrumann]

Sorry for taking so long and thanks for all the work. I really like the result 👍

[kraz]

Just wanted to say thank you for implementing this feature. The config builder will simplify by a lot the dynamic configuration which I am currently using in my project. 👌 🥳 🎉

[ilnytskyi]

Could I ask someone to publish new releases? Last available versions are 1.0.2 for all distributions from December.

[patrickkusebauch]

That would be an ask on @dbrumann . I do not have the privilege to publish a release.

[dbrumann]

Yes, I am planning on a new release for a while. I want to investigate some issues first, but hopefully in 1-2 weeks there will be a 2.0 release

[ostrolucky]

Still no release with this feature :(

[gennadigennadigennadi]

I'm trying to find some time to work on the next release, but I have to understand how we sign those .phar with gpg for phive. Also I have to re-read our documentation/upgrade-infos, because the next release will be version v2.

Maybe I will publish a release with the tag v2.0.0-(beta|rc) in the meantime for the deptrac/deptrac repo and postpone the other distribution options.

@ostrolucky and please remember that the configbuilder is still an experimental feature!
@patrickkusebauch, @dbrumann what to you think guys?

[patrickkusebauch]

@gennadigennadigennadi If we are to release a new Major version (even as a beta/rc), I would like to see:

• ConfigBuilder being the default and yaml deprecated and scheduled to be removed in the next major release (as opposed to yaml being the default and ConfigBuilder being experimental). I have been using it for months now and it works fine. There are minor DevEx features that have open issues already, but they can be worked on in the next major release.
• At least a cursory pass over currently unused/deprecated things and removing them before the next major release. I have a vague memory I have done this a few months back already, but another pass would not hurt.

I can do both this weekend if there is demand.

[patrickkusebauch]

Actually, one more thing. Closing #1243 before a major release would also be nice. Otherwise, we potentially will carry a huge burden for the next major release line.

[gennadigennadigennadi]

At least a cursory pass over currently unused/deprecated things and removing them before the next major release. I have a vague memory I have done this a few months back already, but another pass would not hurt.

You did, but another quick review would be awesome!

ConfigBuilder being the default and yaml deprecated and scheduled to be removed in the next major release (as opposed to yaml being the default and ConfigBuilder being experimental). I have been using it for months now and it works fine. There are minor DevEx features that have open issues already, but they can be worked on in the next major release.

I really like this idea! I'll have to add this to the release notes.

Actually, one more thing. Closing #1243 before a major release would also be nice. Otherwise, we potentially will carry a huge burden for the next major release line.

I don't now if this is doable (right now), becaue it would be necessary to sunset deptrac-shim at the same time. Would you also abandoned the .phar?

---

For me, one of the biggest annoyances is this one: #1287 and i would like to get this solved sooner than later.

[patrickkusebauch]

I don't now if this is doable (right now), becaue it would be necessary to sunset deptrac-shim at the same time. Would you also abandoned the .phar?

I don't have a vested interest in which distribution method we choose. I just want to have only one, so that we don't have the burden of all the build processes and incompatibilities between them (like writing extensions does not work everywhere). I personally use the direct require-dev composer dependency of deptrac, but I do not object picking something else. Completely self-contained distribution with all the dependencies that will not cause any collisions with other installed tools and allows us to pin our own internal dependencies to the version that we need to is probably the best choice.