Remove doctrine

[barryvdh]

See #1510

[KentarouTakeda]

Added fix to #1510,

• Remove Doctrine DBAL
• All tests pass with Laravel >= ^10.38 || 11

The completed code is in my repository.

There are still some points that I would like to carefully check, so it will be a while before I can say "ready for merge", but would you like to submit a draft pull request?

Change set: KentarouTakeda/barryvdh-laravel-ide-helper@laravel-11...laravel-11-remove-doctrine-dbal
CI result: https://github.com/KentarouTakeda/barryvdh-laravel-ide-helper/actions/runs/7817602705/job/21325891898

From what I've tried, the Schema::getXxx() required by the Laravel IDE Helper seems to work starting with Laravel 10.38. The corresponding version in the change set above also complies with that.

[barryvdh]

Sorry didn't see your commit until I fixed some changes. Will compare and check, thanks!

[barryvdh]

I dont think the minimum stability was working before..

[barryvdh]

So what do you think @mfn
Should we target a v3 for this? It doesn't seem too much impact though.

[jameshulse]

Awesome change thanks! I have tested this on a relatively complex Postgres database and it fixes a major issue we had with Doctrine that affected both Laravel and ide-helper previously.

We have a table in two schemas with the same name, but differing types on the id column. Previously Doctrine didn't handle schemas well and would return all columns for all tables matching the name, regardless of schema. So ide-helper ended up with two id columns, one a string, and one an integer. We would sometimes end up with the wrong type information.

I'll be using this dev version until it is fully released.

Edit: I just noticed there isn't handling for the "bpchar" columns type. This is similar to char and should be mapped to PHP string (it is currently mapping to 'mixed'). See postgres docs.

[barryvdh]

Yes added that now

[hafezdivandari]

You may also change config/ide-helper.php to something like this:

/*
|--------------------------------------------------------------------------
| Custom DB type mappings
|--------------------------------------------------------------------------
|
| This setting allow you to map any database type, including custom types that you may have
| created using CREATE TYPE statement or imported using database plugin / extension to a PHP type.
|
| Each key in this array is a `connections.driver` of the `database` config file. Currently valid names are:
| 'pgsql', 'mysql', 'sqlite', 'sqlsrv'
| 
| The value of the array is an array of type mappings. Key is the name of the column type,
| (for example, `jsonb` from Postgres 9.4) and the value is the name of the corresponding PHP type.
|
| So to map `jsonb` to `array` in your models when working with Postgres,
| just add the following entry to the array below:
|
| "pgsql" => [
|     "_int4" => "integer[]",
| ],
|
*/
'custom_db_types' => [

],

You may also check for postgresql (to pgsql) and mssql (to sqlsrv) to avoid breaking change.

PS: We may also check for any type name prefixed by _ on pgsql and map it to array out of the box.

[barryvdh]

So except for the binary -> string change, this doesn't seem to break anything in our current test setup. So LGTM

[mfn]

I'll also try to test this with a big private project ASAP.

[hafezdivandari]

@barryvdh you missed #1512 (comment)?

[mfn]

• First things first: ran this on a private project -> no issues 🎉

• @barryvdh you missed #1512 (comment)?

  This is really a good catch, the keys definitely change (e.g. postgresql -> pgsql, etc. ).

  I didn't actually notice, because I have this:

      'custom_db_types' => [
          'postgresql' => [
              'jsonb' => 'string',
          ],
      ],

  but this change implicitly already supports it, so I wasn't aware of it.

  We should definitely write this down as a kind of a (mini-)migration guide.

• We might also want to point out that people should double check if they can remove doctrine/dbal from their top level dependency in composer. I'm an ide-helper user sinc (OH GOSH!) 8 years and back then adding doctrine/dbal explicitly was required (or necessary for whatever reason) :-}

[barryvdh]

Do we need to group the types by driver still? How does custom mapping work anyways, php handles it the same anyways right? If you want a different type, you should cast it. If it's different for a driver, we should just fix it on our side?

[hafezdivandari]

If you want a different type, you should cast it. If it's different for a driver, we should just fix it on our side?

It seems right.

The only edge case comes to my mind is PostgreSQL array types, we can simply fix that, but it's also string:

DB::statement('create table "test" ("arr" int[])');
DB::statement('insert into "test" ("arr") values (array[1,2,3])');

var_dump(DB::table('test')->value('arr')); // string(7) "{1,2,3}"

So I think we can remove custom_db_types as you said?

[mfn]

Maybe a valid point, though it depends I guess.
There are wild types, or even custom types out there, and being able to quickly have a more useful default mapping gives this a reason.

Example with jsonb which, AFAIK, is pgsql specific: if this PR wouldn't support it out of the box or it wouldn't be in the custom mapping, the default type would be mixed.

With a cast, which performs implicit decoding, it usually becomes array. But not in all cases you need a special cast, so default to string is quit good enough.

Or maybe the root issue is to default to the mixed type. I can imagine that the default actual type usually is string anyway.

But I wouldn't have enough knowledge/data to make that decision.

[hafezdivandari]

Or maybe the root issue is to default to the mixed type. I can imagine that the default actual type usually is string anyway.

I agree. We can assume DB always returns string unless it's explicitly integer, boolean, or float.

[KentarouTakeda]

I have tested this on a private project with 70 tables and 680 columns, including PostgreSQL proprietary types. It produced exactly the same results as the previous version. LGTM.

I agree with treating unknown types as string rather than mixed.Because PDO's implementation is such, we receive those values ​​that Laravel cannot interpret as string.

pgsql:
https://github.com/php/php-src/blob/php-8.3.3/ext/pdo_pgsql/pgsql_statement.c#L673-L674

mysql:
https://github.com/php/php-src/blob/php-8.3.3/ext/pdo_mysql/mysql_statement.c#L823-L825

sqlite:
https://github.com/php/php-src/blob/php-8.3.3/ext/pdo_sqlite/sqlite_statement.c#L331-L338

sqlsrv(odbc):
https://github.com/php/php-src/blob/php-8.3.3/ext/pdo_odbc/odbc_stmt.c#L645

[barryvdh]

Yes removed them.

[mfn]

Re-checked my private project (pgsq, 100+ tables, 1000k+ columns) => s'all good :-}

Suggestion for the CHANGELOG:

### Changed
- Use of doctrine/dbal [#1512 / barryvdh](https://github.com/barryvdh/laravel-ide-helper/pull/1512)
  With this functionality gone, a few changes have been made:
  - support for custom datatypes has been dropped (config `custom_db_types`)
    unknown data types default to `string` now and to fix the type, add a proper cast in Eloquent
  - You _might_ have top-level dependency on doctrine/dbal. This may have been in the past due to ide-helper, we suggest to check if you still need it and remove it otherwise

Also 👍🏼 for cutting a 3.0 of it. Do we want to push out 3.0 directly or make a preview release?

[barryvdh]

Yeah bit conflicted about the 3.0 vs 2.x. It technically isn't a BC break, only if people are relying on Doctrine because of us.
I only think it's a bit annoying to force everyone to upgrade, but they would probably check their dependencies for Laravel 11 anyway.

But probably better to force the upgrade in 3.x, as it will coincide with Laravel dropping it anways. https://laravel.com/docs/master/upgrade#doctrine-dbal-removal

[barryvdh]

Okay changed the branch alias to 3.0-dev

[mfn]

Thanks @barryvdh , really great work so we can move forward here, thank you (et al) 🙏🏼