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

using /vendor/ as a layer #506

Closed
staabm opened this issue Mar 3, 2021 · 17 comments
Closed

using /vendor/ as a layer #506

staabm opened this issue Mar 3, 2021 · 17 comments

Comments

@staabm
Copy link
Contributor

staabm commented Mar 3, 2021

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
Copy link
Contributor

smoench commented Mar 3, 2021

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

@clxmstaab
Copy link
Contributor

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
Copy link

MartinMystikJonas commented Mar 4, 2021

@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
Copy link
Contributor

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

worked for me.

@smoench smoench closed this as completed Mar 8, 2021
@MGatner
Copy link

MGatner commented Jun 30, 2021

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
Copy link
Collaborator

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
Copy link

MGatner commented Jun 30, 2021

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
Copy link
Collaborator

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
Copy link

MGatner commented Jul 1, 2021

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

@MGatner
Copy link

MGatner commented Jul 2, 2021

@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
Copy link
Collaborator

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
Copy link

MGatner commented Jul 2, 2021

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
Copy link
Collaborator

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
Copy link

MGatner commented Jul 2, 2021

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
Copy link

MGatner commented Jul 2, 2021

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
Copy link
Collaborator

@MGatner that is my bad, bugfix incoming: #618

@MihaiNeagu
Copy link

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?

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

8 participants