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

Is it possible to disallow direct extension, but allow indirect extension? #1186

Closed
pckuijper opened this issue May 17, 2023 · 12 comments
Closed

Comments

@pckuijper
Copy link

I wanna start of with by thanking everybody working on this tool for what it makes possible! It's great and helps me and my team to build better software in guidance with our architectural rules.

My question comes from a want to only allow our internal code to make use of our own exception layer.
The setup is roughly as follows:

final class specificException extends CompanyInputException
{}

class CompanyInputException extends CompanyRuntimeException
{}

Abstract class CompanyRuntimeException extends \RuntimeException
{}

final class SomeService
{
    public function foo()
    {
        throw new \RuntimeException(); // This should not be allowed
    }
}

final class AnotherService
{
    public function foo()
    {
        throw new CompanyRuntimeException(); // This should be allowed
    }
}

The config I'm trying to work with is roughly follows

deptrac:
  paths:
    - src

  analyser:
    types:
      - "class"

  layers:
    - name: Base_Php_Exceptions
      collectors:
        - type: className
          value: ^Exception$
        - type: className
          value: ^LogicException$
        - type: className
          value: ^BadFunctionCallException$
        - type: className
          value: ^BadMethodCallException$
        - type: className
          value: ^DomainException$
        - type: className
          value: ^InvalidArgumentException$
        - type: className
          value: ^LengthException$
        - type: className
          value: ^OutOfRangeException$
        - type: className
          value: ^RuntimeException$
        - type: className
          value: ^OutOfBoundsException$
        - type: className
          value: ^OverflowException$
        - type: className
          value: ^RangeException$
        - type: className
          value: ^UnderflowException$
        - type: className
          value: ^UnexpectedValueException$

    - name: Company_Base_Exceptions
      collectors:
        - type: className
          value: ^Company\\Exception

    - name: Company
      collectors:
        - type: bool
          must:
             - type: className
                value: Company\\.*
          must_not:
             - type: className
                value: Company\\Exception\\.*
  ruleset:
    Company_Base_Exceptions:
      - Base_Php_Exceptions

    Company:
      - Company_Base_Exceptions

The issue I am running into, is that something in the Company layer is using the Company_Php_Exceptions layer, which itself relies on the Base_Php_Exceptions, a violation is raised because it checks the whole dependency graph and not just the first layer.

I've tried using the InheritanceLevel collector, but to no avail.

Now I'm starting to wonder if this is possible at all using Deptrac, or if I have to rely on other tooling.

Any thoughts, or guidance is much appreciated!

@patrickkusebauch
Copy link
Collaborator

No need to look for another tool. It is possible. Give me a few hours to finish at work and I will post a solution.

@patrickkusebauch
Copy link
Collaborator

Can you provide me with the text of the Violation as shown in the console? It would help me craft the code for you. Or the repo in question, if it is public?

@pckuijper
Copy link
Author

The violation I'm getting is this one

  Violation   Company\Infrastructure\SomeClass must not depend on RuntimeException (Base_Php_Exceptions)
              Company\Infrastructure\SomeClass::12 ->
              Company\Exception\SystemException::13 ->
              RuntimeException::24
              src/Exception/CompanyRuntimeException.php:24

Unfortunately I cannot share the repository, but if this isn't enough information, I can recreate the issue in a separate repo

@patrickkusebauch
Copy link
Collaborator

The short answer is - you can use a custom Analyser Event subscriber to skip those violations. Look at the docs: https:/qossmic/deptrac/blob/main/docs/extending_deptrac.md

Deptrac even already does this internally, if you look here: https:/qossmic/deptrac/blob/main/internal/deptrac/IgnoreDependenciesOnContract.php and how it is registered here:

$services->set(IgnoreDependenciesOnContract::class)->tag('kernel.event_listener', ['event' => ProcessEvent::class]);
or here:
- class: Internal\Qossmic\Deptrac\IgnoreDependenciesOnContract

You just need to match the violation in question and stop propagation right there.

@patrickkusebauch
Copy link
Collaborator

Small reproducing repo would certainly be helpful.

@pckuijper
Copy link
Author

Here is a small example project with this structure reproducing the problem.

https:/pckuijper/custom-exceptions

I'm trying to figure out how to set up what you describe in the short answer, but unfortunately to no avail just yet.

@patrickkusebauch
Copy link
Collaborator

No promises, but give me 24 hours.

@patrickkusebauch
Copy link
Collaborator

Provided in pckuijper/custom-exceptions#1

@pckuijper
Copy link
Author

pckuijper commented May 19, 2023

Thanks for helping with implementing this! It was a great help.

One final issue I'm running into is quite likely related to the version of deptrac we're using (0.24) due to php + symfony constraints.

It looks like the code should still work in the same way and I'm trying to tag my event listener by using

services:
  - class: Company\Tests\Framework\StaticAnalysis\Deptrac\AllowCustomExceptionsRule
    tags:
      - { name: kernel.event_listener, event: Qossmic\Deptrac\Contract\Analyser\ProcessEvent }

Copied over from https:/qossmic/deptrac/blob/0.24.0/deptrac.yaml
But while it doesn't seem to get executed.

Is there any known issues with that version and extending deptrac with custom services?

I've updated my example project with version 0.24 (pckuijper/custom-exceptions@038846f)

@patrickkusebauch
Copy link
Collaborator

For event listener it might not get executed if there was a listener with higher priority that was executed before yours with the default priority that has called stopPropagation on the event. That how Symfony works AFAIK. I think some of the Violation creating events do that.

@patrickkusebauch
Copy link
Collaborator

patrickkusebauch commented May 19, 2023

Hi, so I looked into it, and yes, DependencyType has been introduced in deptrac v1.0.0 (see dc928f7) and is not available in v0.2.4. SO that is certainly a problem. I have updated what I could in your repo in a PR: pckuijper/custom-exceptions#2

PHP Fatal error:  Uncaught Error: Call to undefined method Qossmic\Deptrac\Core\Dependency\Dependency::getType() in /home/patrick/PhpstormProjects/custom-exceptions/src/CustomExceptionRule.php:32

@pckuijper
Copy link
Author

pckuijper commented May 22, 2023

I have found a "workaround", it's probably not as optimal as it could be but it's good enough for us for now!

I'll leave the repository up in case this is of any help to anybody in the future. The main branch has a working setup with the current latest version of deptrac ^1.0 and there is a branch for version 0.24

Repo:
https:/pckuijper/custom-exceptions (example with version ^1)
https:/pckuijper/custom-exceptions/tree/fix-for-version-0.24

Thanks a lot for your help!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants