fumeapp / modeltyper

Generate TypeScript interfaces from Laravel Models
MIT License
136 stars 16 forks source link

[BUG] Using non-standard database driver for one model causes the command to fail. #69

Open nonetallt opened 9 months ago

nonetallt commented 9 months ago

What happened?

The Problem

I specified a "clickhouse" connection for one of my models using the laravel clickhouse db driver. Now whenever I run the model:typer command, it results in the following error in the output:

   BadMethodCallException 

  Method PhpClickHouseLaravel\Connection::getDoctrineDriver does not exist.

  at vendor/laravel/framework/src/Illuminate/Macroable/Traits/Macroable.php:112
    108▕      */
    109▕     public function __call($method, $parameters)
    110▕     {
    111▕         if (! static::hasMacro($method)) {
  ➜ 112▕             throw new BadMethodCallException(sprintf(
    113▕                 'Method %s::%s does not exist.', static::class, $method
    114▕             ));
    115▕         }
    116▕

  i   Bad Method Call: Did you mean PhpClickHouseLaravel\Connection::getDoctrineColumn() ? 

      +41 vendor frames 

  42  artisan:35
      Illuminate\Foundation\Console\Kernel::handle()

It seems pretty obvious that the underlying model:show command relies on getDoctrineDriver(), which might not exist for all database drivers.

The Solution

It seems like try-catching for this scenario is not viable, considering the highly generic type of the BadMethodCallException, where catching it might result in unintentionally hiding errors with a different cause.

The first solution that comes to mind would be to implement a feature to ignore certain models (or models with a certain connection type). This should be very straightforward given that the GetModels action already supports exclusion of certain models (the parameters are simply not utilized currently).

Model exclusion could be implemented as either config or command option. Do you happen to have any preference for either approach or would you suggest and entirely different solution to begin with?

Expected Behavior

Model types are generated as normal.

Steps To Reproduce

1) Install https://github.com/glushkovds/phpclickhouse-laravel 2) Create a config/database entry for 'clickhouse' 3) Create a new model. Set protected $connection = 'clickhouse' 4) Run model:typer command

nonetallt commented 9 months ago

On an unrelated note, is the packagist auto-update hook broken?

Version 2.2.8 is still not in https://packagist.org/packages/fumeapp/modeltyper where version 2.2.7 is the newest stable release.

tcampbPPU commented 9 months ago

On an unrelated note, is the packagist auto-update hook broken?

Version 2.2.8 is still not in https://packagist.org/packages/fumeapp/modeltyper where version 2.2.7 is the newest stable release.

I fixed the release 2.2.9 should be latest

tcampbPPU commented 9 months ago

The Problem

It seems pretty obvious that the underlying model:show command relies on getDoctrineDriver(), which might not exist for all database drivers. ...

This might be more of a model:show laravel command issue as they support which drivers can be used with it. This being a custom driver im not entirely sure. I would be curious if there are any issues out on the framework for this type is issue with unsupported drivers.

My initial thought would be to filter only models with supported drivers as we need Laravel to do its thing before we can try to convert it

github-actions[bot] commented 8 months ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.