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

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

Closed
Radiergummi opened this issue Nov 16, 2023 · 10 comments

Comments

@Radiergummi
Copy link

  • Package Version: 8.11.3
  • Laravel Version: 10.30.1
  • PHP Version: 8.2.7

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:

  • Canvas should only apply modifications if installed as a direct dependency (or part of workbench).
  • If that isn't possible, the help output of overridden commands should include a message indicating that a modified version is running.

Steps to reproduce:

  1. Install a package that depends on canvas
  2. Observe make commands being replaced
@crynobone
Copy link
Member

Please share your composer.json

@crynobone
Copy link
Member

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

@Radiergummi
Copy link
Author

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

Radiergummi commented Nov 16, 2023

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

This is apparently on purpose - as per psalm/psalm-plugin-laravel#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
Copy link
Member

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

@crynobone
Copy link
Member

Also, I cannot find ApplicationHelper mentioned in the thread

@crynobone
Copy link
Member

Submitted psalm/psalm-plugin-laravel#359

@Radiergummi
Copy link
Author

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

@crynobone
Copy link
Member

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

"extra": {
    "laravel": {
        "dont-discover": [
            "orchestra/canvas"
        ]
    }
},

@Radiergummi
Copy link
Author

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

This issue was closed.
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