orchestral / canvas

Code Generators for Laravel Applications and Packages
https://packagist.org/packages/orchestra/canvas
MIT License
187 stars 12 forks source link

Canvas silently replaces make commands when installed as a transitive dependency #30

Closed Radiergummi closed 8 months ago

Radiergummi commented 8 months ago

Description:

I noticed my make: commands had a --preset flag with the semi-helpful description Preset used when generating $X that I couldn't find any documentation for, and that didn't appear in the framework's make commands.
Long story short, I discovered the commands had been overridden by canvas, which had been installed via Psalm's Laravel plugin:

composer why orchestra/canvas -t
orchestra/canvas v8.11.3 Code Generators for Laravel Applications and Packages
├──orchestra/canvas-core v8.10.0 (conflicts orchestra/canvas <8.11.0) (circular dependency aborted here)
└──orchestra/workbench v8.0.0 (requires orchestra/canvas ^8.11)
   └──orchestra/testbench v8.14.1 (requires orchestra/workbench ^1.0 || ^8.0)
      └──psalm/plugin-laravel v2.8.0 (requires orchestra/testbench ^7.19 || ^8.0)
         └──my/app dev-main (requires (for development) psalm/plugin-laravel ^2.8)

I understand how and why this works the way it does from the service discovery side, but it still feels way too far-reaching to silently override the commands without any hints to the developer.
I should not have to expect side effects to the way my application works from installing an apparently unrelated third-party dependency.

My suggestion is thus:

Steps to reproduce:

  1. Install a package that depends on canvas
  2. Observe make commands being replaced
crynobone commented 8 months ago

Please share your composer.json

crynobone commented 8 months ago

I believe psalm/plugin-laravel should require orchestra/testbench as required-dev and not required.

Radiergummi commented 8 months ago

Please share your composer.json

Here goes:

{
    "name": "my/app",
    "type": "project",
    "require": {
        "php": "^8.2",
        "firebase/php-jwt": "^6.9",
        "guzzlehttp/guzzle": "^7.8",
        "guzzlehttp/psr7": "^2.6",
        "laravel/framework": "^10.10",
        "laravel/octane": "^2.1",
        "laravel/tinker": "^2.8",
        "psr/http-factory": "^1.0",
        "sentry/sentry-laravel": "^3.8",
        "spatie/laravel-fractal": "^6.0",
        "spatie/laravel-json-api-paginate": "^1.13",
        "spatie/laravel-query-builder": "^5.6",
        "spatie/laravel-route-attributes": "^1.19",
        "symfony/cache": "^6.3",
        "tpetry/laravel-postgresql-enhanced": "^0.32.0"
    },
    "require-dev": {
        "fakerphp/faker": "^1.9.1",
        "laravel/pint": "^1.0",
        "magentron/laravel-blade-lint": "^2.0",
        "mockery/mockery": "^1.4.4",
        "nunomaduro/collision": "^7.0",
        "phpunit/phpunit": "^10.1",
        "psalm/plugin-laravel": "^2.8",
        "spatie/laravel-ignition": "^2.0",
        "vimeo/psalm": "^5.15"
    },
    "autoload": {
        "psr-4": {
            "App\\": "app/",
            "Database\\Factories\\": "database/factories/",
            "Database\\Seeders\\": "database/seeders/",
            "Database\\Utilities\\": "database/utilities/"
        },
        "files": [
            "bootstrap/functions.php"
        ]
    },
    "autoload-dev": {
        "psr-4": {
            "Tests\\": "tests/"
        }
    },
    "extra": {
        "laravel": {
            "dont-discover": []
        }
    },
    "config": {
        "optimize-autoloader": true,
        "preferred-install": "dist",
        "sort-packages": true,
        "allow-plugins": {
            "pestphp/pest-plugin": true,
            "php-http/discovery": true
        }
    },
    "minimum-stability": "stable",
    "prefer-stable": true
}
Radiergummi commented 8 months ago

I believe psalm/plugin-laravel should require orchestra/testbench as required-dev and not required.

This is apparently on purpose - as per https://github.com/psalm/psalm-plugin-laravel/issues/130:

orchestra/testbench is currently a dependency for support of packages. Developers who create third party laravel packages need to be able to run static analysis, but they don't necessarily have an app to boot up. Testbench currently provides us one that we can easily use in those scenarios.

I'm not entirely convinced by their reasoning, though...

crynobone commented 8 months ago

The last response has the best solution, require orchestra/testbench-core instead

crynobone commented 8 months ago

Also, I cannot find ApplicationHelper mentioned in the thread

crynobone commented 8 months ago

Submitted psalm/psalm-plugin-laravel#359

Radiergummi commented 8 months ago

Hey, thank for looking into this and even creating the PR! Much appreciated 🫶

crynobone commented 7 months ago

Since it doesn't seem that the PR will be accepted soon you can add the following:

"extra": {
    "laravel": {
        "dont-discover": [
            "orchestra/canvas"
        ]
    }
},
Radiergummi commented 7 months ago

The discussion on that PR is indeed a bit… concerning. Thank you for the suggestion though!