qossmic / deptrac

Keep your architecture clean.
https://qossmic.github.io/deptrac
MIT License
2.62k stars 136 forks source link

Unable to use custom `EventSubscriberInterface` in depfile since 2.0 as those are prefixed in the phar #1417

Open janedbal opened 4 months ago

janedbal commented 4 months ago

I tried upgrading our customized deptrac setup to 2.0 (we have custom baseline implementation) and found that you started prefixing some extension points like EventSubscriberInterface, I believe this only complicates extendability of this library.

app@5fa22c221664:/app$ vendor/bin/deptrac

In RegisterListenersPass.php line 103:

  [DEPTRAC_202404\Symfony\Component\DependencyInjection\Exception\InvalidArgumentException]                                                 
  Service "baselineViolationHandler" must implement interface "DEPTRAC_202404\Symfony\Component\EventDispatcher\EventSubscriberInterface".  

Exception trace:
  at /app/vendor/qossmic/deptrac/vendor/symfony/event-dispatcher/DependencyInjection/RegisterListenersPass.php:103
 DEPTRAC_202404\Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass->process() at /app/vendor/qossmic/deptrac/vendor/symfony/dependency-injection/Compiler/Compiler.php:70
 DEPTRAC_202404\Symfony\Component\DependencyInjection\Compiler\Compiler->compile() at /app/vendor/qossmic/deptrac/vendor/symfony/dependency-injection/ContainerBuilder.php:652
 DEPTRAC_202404\Symfony\Component\DependencyInjection\ContainerBuilder->compile() at /app/vendor/qossmic/deptrac/src/Supportive/DependencyInjection/ServiceContainerBuilder.php:84
 Qossmic\Deptrac\Supportive\DependencyInjection\ServiceContainerBuilder->build() at /app/vendor/qossmic/deptrac/src/Supportive/Console/Application.php:64
 Qossmic\Deptrac\Supportive\Console\Application->doRun() at /app/vendor/qossmic/deptrac/vendor/symfony/console/Application.php:162
 DEPTRAC_202404\Symfony\Component\Console\Application->run() at /app/vendor/qossmic/deptrac/bin/deptrac:24
 include() at /app/vendor/bin/deptrac:120

Defined as

services:
  baselineViolationHandler:
    class: ShipMonk\Rules\Deptrac\EventHandler\BaselineViolationHandler
    arguments:
      $baseline: '@baseline'
    tags:
      - { name: kernel.event_subscriber }
class BaselineViolationHandler implements \Symfony\Component\EventDispatcher\EventSubscriberInterface {
    // ...
}
gennadigennadigennadi commented 4 months ago

I don’t think that the internal dependencies have ever been a public api. Properly a better solution would be to have a official extension point which is provided by Deptrac itself.

another option could be to prefix the dependencies with a static prefix. And people could still use those.

janedbal commented 4 months ago

I understand. I just think that Deptrac was very nicely customizable and extendable thanks to its event-driven design and the fact you have access to its DIC. That allowed us some neat customizations we needed. And now it got a bit worse :)

gennadigennadigennadi commented 4 months ago

With using the Symfony event from Deptrac your project adds a dependency. And if Deptracs decides to drop the Symfony stuff your code will break.

long story short: Deptrac should define extension points and document them. Maybe that’s already the case @patrickkusebauch ?

gennadigennadigennadi commented 4 months ago

By the way, with the new release model, I can release new version quicker! And I will try do release Deptrac more often!

My next goal is to figure out how we deploy the documentation 😅.

janedbal commented 4 months ago

Yeah, I know the risk and I'm willing to take it. Obviously I'm doing non-standard thing, I just wanna let you know that there are deptrac users that utilize its internal stuff.

patrickkusebauch commented 3 months ago

I think a nice middle ground would be to have a static prefix for the Symfony Event Dispatcher, something like Deptrac\Symfony\Component\EventDispatcher\EventSubscriberInterface. That way the users can depend on a consistent name to extend that would not break between releases.

@janedbal do you think that would be good enough for your use case?

gennadigennadigennadi commented 3 months ago

I’ve merged ‘Deptrac_Internal’ as the namespace prefix for Deptrac a dependencies.

See https://github.com/qossmic/deptrac-src/pull/48/files. There is not a release wirh the static prefix, so there is still the possibility to change it.

janedbal commented 3 months ago

If you insist on prefixing, then static one is definitelly better. Thank you