qossmic / deptrac

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

using `/vendor/` as a layer #506

Closed staabm closed 3 years ago

staabm commented 3 years ago

we want to minimize dependencies on external libraries (those installed via composer), or at least allow using those externals only from certain layers.

from a architecture perspective we want 3rd party code only allow within the Integration layer.

we try to use something like this (simplified):

paths:
  - app/
layers:
  - name: Controller
    collectors:
      - type: className
        regex: .*Controller.*
  - name: Business
    collectors:
      - type: className
        regex: .*\\business\\.*
  - name: BusinessException
    collectors:
      - type: className
        regex: .*\\exception\\.*
  - name: Vendor
    collectors:
      - type: directory
        regex: .*/vendor/.*
ruleset:
  Controller:
    - ViewModel
  Business:
    - BusinessException
  Integration:
    - Business
    - Vendor

we are wondering why we don't see a violation for the following code

// app/portal/controllers/AuthenticationController.php
<?php

class AuthenticationController extends ApplicationController
{
    public function createArchitectureErrorForTesting(): void
    {
        $test = (new \ZxcvbnPhp\Zxcvbn())->passwordStrength('xyt');
    }
}

the mentioned lib is installed via composer.json

{
...
    "require" : {
        "ext-json": "*",
        "ext-soap": "*",
        "bjeavons/zxcvbn-php": "^1.2"
    },
...

PS: using the regex on /vendor/ a few packages would be whitelisted via negative lookahead or similar (e.g. framework packages)

smoench commented 3 years ago

You need to add the vendor dir to paths otherwise deptrac doesn't know where a vendor class is located.

clxmstaab commented 3 years ago

You need to add the vendor dir to paths otherwise deptrac doesn't know where a vendor class is located.

thx for the suggestion. this means deptrac would then analyze all dependencies regarding our ruleset, which is not useful. is there a way to take a path into account without analysing it?

MartinMystikJonas commented 3 years ago

@clxmstaab I would recommend to not trying to add whole /vendor/ directory but specify Vendor layer as list of allowed vendor namespaces. We use it this way:

layers:
  - name: Vendor
    collectors:
      - type: className
        regex: ^(Metis|Nette|Tracy|Drahak|Kdyby|GuzzleHttp)\\.*

It has added benefit of detecting unwanted dependencies on something that is added only as transitive dependency required by your direct dependencies.

clxmstaab commented 3 years ago

thx for the hint. this totally makes sense and might be something worth noting in the readme.

worked for me.

MGatner commented 3 years ago

Would @MartinMystikJonas 's suggestion be the way to include classes in a layer that we wanted to exclude but not check? For example, in a framework application you might not want app/Models/UserModel.php to use anything from app/Libraries/ but also not from vendor/org/framework/src/Libraries/. BUT, I don't care about violations from anything in vendor/ itself.

dbrumann commented 3 years ago

Yes, personally I think that's a good way to solve this.

For example you could have two layers App Libraries and External Liberaries and then specify which layers may access each one, e.g. UserModel may not use either, but maybe App Libraries can use External Libraries.

If for whatever reason you run into a problem with that approach, feel free to open a separate issue with your depfile (or just the relevant parts) so we can look at the concrete use case.

MGatner commented 3 years ago

Thanks @dbrumann. I think that isn't quite what I need but it is helpful. The problem is that the layer is all the same; maybe a simplified example will help:

paths:
  # Actual project code:
  - ./src
  # Additional components project code should not violate
  - ./vendor/codeigniter4/codeigniter4/system
  - ./vendor/sparks
exclude_files:
  - '#.*test.*#i'
layers:
  - name: Model
    collectors:
      - type: className
        regex: .*[A-Za-z]+Model.*
  - name: Controller
    collectors:
      - type: className
        regex: .*\/Controllers\/.*
  - name: Service
    collectors:
      - type: className
        regex: .*Services.*
ruleset:
  Controller:
    - Model
    - Service
  Model:
    - Service
  Service:
    - Model

In this case, Services may exist in any of three places: my project src/, the framework's system, or any of the framework modules from sparks. I want to enforce that my project's Services do not access Controllers from any of the collections, but I don't want reports or failures if a module from sparks violates the same rule.

patrickkusebauch commented 3 years ago

Bool collector is your friend here. It will be a little bit of duplication but if you split your services into 2 like this:

layers:
  - name: Service 1
    collectors:
      - type: bool
        must:
          - type: className
            regex: .*Services.*
          - type: directory
            regex: src/.*
  - name: Service 2
    collectors:
      - type: bool
        must:
          - type: className
            regex: .*Services.*
          - type: directory
            regex: vendor/sparks/.*
      - type: bool
        must:
          - type: className
            regex: .*Services.*
          - type: directory
            regex: vendor/codeigniter4/codeigniter4/system/.*

You should be able the make the desired rulesets.

MGatner commented 3 years ago

Awesome! Thanks @patrickkusebauch I will give that a shot.

MGatner commented 3 years ago

@patrickkusebauch Is there a way to "allow all"? I read through docs and issues and did not see this. The problem with defining them as separate layers is:

By default, all dependencies between layers are forbidden!

Which means any "__ 2" will cause violations without explicitly defining all of them.

patrickkusebauch commented 3 years ago

No, there is no allow all but there are transitive dependencies when one layer can copy dependencies of another layer.

Unfortunately, the docs were lost in the refactor, I already created issue #617.

In the meantime, you can find the original docs for the feature in #579 changed files.

MGatner commented 3 years ago

So if I understand this correctly... I would do something like the following to "allow all" for my vendor layer?

 layers:
   - name: Widget
   - name: Foo
   - name: Bar
   - name: Baz
   - name: Vendor Widget
   - name: Vendor Foo
   - name: Vendor Bar
   - name: Vendor Baz
 ruleset:
   Vendor Widget:
     - Vendor Foo
     - Vendor Bar
     - Vendor Baz
   Vendor Foo:
     - +Vendor Widget
   Vendor Bar:
     - +Vendor Widget
   Vendor Baz:
     - +Vendor Widget
patrickkusebauch commented 3 years ago

This is eqivalent to:

 layers:
   - name: Widget
   - name: Foo
   - name: Bar
   - name: Baz
   - name: Vendor Widget
   - name: Vendor Foo
   - name: Vendor Bar
   - name: Vendor Baz
 ruleset:
   Vendor Widget:
     - Vendor Foo
     - Vendor Bar
     - Vendor Baz
   Vendor Foo:
     - Vendor Foo
     - Vendor Bar
     - Vendor Baz
   Vendor Bar:
     - Vendor Foo
     - Vendor Bar
     - Vendor Baz
   Vendor Baz:
     - Vendor Foo
     - Vendor Bar
     - Vendor Baz

Is that what you want?

MGatner commented 3 years ago

Yes! I want everything in my vendor/ folder allowed but still be able to restrict use of it from my source code. I will try it.

MGatner commented 3 years ago

Is this already a part of 0.14.0? It does not appear to recognize the syntax:

Configuration can not reference rule sets with unknown layer names, got "+Vendor Model" as unknown.

Tried it in quotes too, to no avail.

Edit: Relevant portion of depfile

  # Ignore anything in the Vendor layers
  Vendor Model:
    - Service
    - Vendor Controller
    - Vendor Config
    - Vendor Entity
    - Vendor View
  Vendor Controller:
    - +Vendor Model
  Vendor Config:
    - +Vendor Model
  Vendor Entity:
    - +Vendor Model
  Vendor View:
    - +Vendor Model
patrickkusebauch commented 3 years ago

@MGatner that is my bad, bugfix incoming: https://github.com/qossmic/deptrac/pull/618

MihaiNeagu commented 8 months ago

Regarding the treatment of the vendor as a layer, I have the following depfile

parameters:
  ProjectName: MyProject

deptrac:
  paths:
    - ./src/MyProject
    - ./vendor
  exclude_files:
    - '#.*test.*#'
  layers:
    - name: DomainLayer
      collectors:
        - type: classNameRegex
          value: '#^%ProjectName%\\Domain.*#'
    - name: ApplicationLayer
      collectors:
        - type: classNameRegex
          value: '#^%ProjectName%\\Application.*#'
    - name: InfrastructureLayer
      collectors:
        - type: classNameRegex
          value: '#^%ProjectName%\\Infra.*#'

    - name: InternalPackagesLayer
      collectors:
        - type: directory
          value: 'vendor/symfony/*'
        - type: directory
          value: 'vendor/doctrine/*'
        - type: directory
          value: 'vendor/some-package/*'

    - name: ExternalPackagesLayer
      collectors:
        - type: bool
          must:
            - type: directory
              value: 'vendor/*'
          must_not:
            - type: layer
              value: InternalPackagesLayer

  ruleset:
    InternalPackagesLayer:
      - ExternalPackagesLayer
    ExternalPackagesLayer:
      - InternalPackagesLayer
    InfrastructureLayer:
      - DomainLayer
      - ApplicationLayer
      - InternalPackagesLayer
      - ExternalPackagesLayer
    ApplicationLayer:
      - DomainLayer
      - InternalPackagesLayer
    DomainLayer:
      - InternalPackagesLayer

Event though InternalPackagedLayer is set to depend on ExternalPackagesLayer and viceversa, somehow deptrac returns errors for violations of dependencies between these two layers.

Some examples:

Reason      InternalPackagesLayer                                                                                                                                                 
  Violation   Symfony\Bundle\FrameworkBundle\Test\BrowserKitAssertionsTrait must not depend on PHPUnit\Framework\ExpectationFailedException (ExternalPackagesLayer)                 
  Violation   Symfony\Bundle\FrameworkBundle\Test\BrowserKitAssertionsTrait must not depend on PHPUnit\Framework\ExpectationFailedException (ExternalPackagesLayer)                 
  Violation   Symfony\Bundle\FrameworkBundle\Test\KernelTestCase must not depend on PHPUnit\Framework\Reorderable (ExternalPackagesLayer)                                           
  Violation   Symfony\Bundle\FrameworkBundle\Test\KernelTestCase must not depend on PHPUnit\Framework\SelfDescribing (ExternalPackagesLayer)                                        
  Violation   Symfony\Bundle\FrameworkBundle\Test\WebTestCase must not depend on PHPUnit\Framework\Reorderable (ExternalPackagesLayer)                                              
  Violation   Symfony\Bundle\FrameworkBundle\Test\WebTestCase must not depend on PHPUnit\Framework\SelfDescribing (ExternalPackagesLayer)                                           

and

Reason      ExternalPackagesLayer                                                                                                           
  Violation   GuzzleHttp\Utils must not depend on Symfony\Polyfill\Intl\Idn\Idn (InternalPackagesLayer)                                       
  Violation   GuzzleHttp\Utils must not depend on Symfony\Polyfill\Intl\Idn\Idn (InternalPackagesLayer)                                       
  Violation   GuzzleHttp\Utils must not depend on Symfony\Polyfill\Intl\Idn\Idn (InternalPackagesLayer)                                       
  Violation   GuzzleHttp\Utils must not depend on Symfony\Polyfill\Intl\Idn\Idn (InternalPackagesLayer)                                       
  Violation   PHPStan\PharAutoloader must not depend on Symfony\Polyfill\Php80\Php80 (InternalPackagesLayer)                                  
  Violation   PHPStan\PharAutoloader must not depend on Symfony\Polyfill\Mbstring\Mbstring (InternalPackagesLayer)                            
  Violation   PHPStan\PharAutoloader must not depend on Symfony\Polyfill\Intl\Normalizer\Normalizer (InternalPackagesLayer)                   
  Violation   PHPStan\PharAutoloader must not depend on Symfony\Polyfill\Php73\Php73 (InternalPackagesLayer)                                  
  Violation   PHPStan\PharAutoloader must not depend on Symfony\Polyfill\Intl\Grapheme\Grapheme (InternalPackagesLayer)                       
  Violation   PHPStan\PharAutoloader must not depend on Symfony\Polyfill\Php81\Php81 (InternalPackagesLayer)                                  

What am I doing wrong here?