Internal call implementation

[patrickkusebauch]

Fixes #538

Tests are implemented by making make deptrac passing. I am unable to write an e2e test from scratch.

[dbrumann]

Thank you, again!

[staabm]

ohhh I like it. thank you sir!

[BafS]

Great addition but is there a way to disable it? For legacy projects

[patrickkusebauch]

@BafS no, there is not. New 0.x version (BC breaking version) means new stricter rules and often requires some work on the side of the user to make it work.

[BafS]

Okay that work, thanks for the feedback!

[patrickkusebauch]

@BafS But if you really need to, we can work together on extending the current functionality to make it conditional.

[BafS]

@patrickkusebauch thank you!
I did the change and excluded some files but I think the issue comes from the fact that @internal is sometimes used to say "this is for internal use only, not production" (for example it could still be used in dev commands, fixtures, tests) or to simply add a notice for internal devs.

Looking at the PSR description we have

Authors MAY use this tag to indicate that an element with public visibility should be regarded as exempt from the API

https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc-tags.md#56-internal

So it's not purely about the boundaries I think, I feel that the option could make sense

[patrickkusebauch]

@BafS Here is how I would go about it. There is a way to write extensions to deptrac in you look at PR #867. In this particular case what makes sense to me is to create a ProcessEventListener akin to what I described here: #625 (comment).

You have the event with the reference to both sides of the dependency, so you can make targeted checks like:

• if the source of the dependency is a test/dev code
• and the target of the dependency has the @internal annotation

Drop the dependency.

I think this approach is better than doing a blank disable of the feature.

WDYT? Would that work for you? I am happy to discuss in deptrac Slack channel: https://symfony-devs.slack.com/archives/C036EU1GZS9

[BafS]

That sounds good! I'll have a look there, thanks a lot for your feedback, really helpful

[dbrumann]

I will also try making this configurable via the deptrac.yaml file over the weekend.

Extensions are not yet actively supported, because I wasn't able to verify if they work with the phar from phive or deptrac-shim, which are still the recommended ways of installing Deptrac. Also, I might make a few changes to the API before making extensions public. That's why I don't want to "encourage" writing your own extensions yet.

[vudaltsov]

Making this configurable would be great. I have a separate deptrac file that checks that tests can reference src and not the other way around. I would have disabled @internal support in that particular file.

[brettmc]

Hi, just a follow-up/feedback here. Would it be possible when reporting "internal" violations to hint at the cause? eg "X must not depend on @internal Y"? I think the feature is great, but I assumed it was a bug in deptrac because it was a dependency that should have been allowed (if not for the internal annotation)

[patrickkusebauch]

@dbrumann what do you think about reworking the Qossmic\Deptrac\Core\Analyser\EventHandlers to be more granular and allowing to disable some of them+ability to add new ones?

This could replace Qossmic\Deptrac\Core\Dependency\Emitter\DependencyEmitterInterface and related configuration. We would have applied all emitters all the time and instead configure them on the Qossmic\Deptrac\Core\Analyser\EventHandler level. Those handlers then could provide a description as to why such a Rule has been applied.

[dbrumann]

what do you think about reworking the Qossmic\Deptrac\Core\Analyser\EventHandlers to be more granular and allowing to disable some of them+ability to add new ones?

Sounds good, but can you explain in more detail what you mean? In theory you can already disable them, by removing the kernel.event_subscriber-tag, but that requires both knowledge of Deptrac-internals and Symfony-DI. Making this configurable would be better. Similarly, you can add new ones by registering them as listener/subscriber in the configuration. So, I'm not sure what the change would look like.

I'm all on board for replacing the DependecyEmitters with Events.