Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

Improve Schema Grammar #1527

Open
driesvints opened this issue Feb 19, 2019 · 9 comments
Open

Improve Schema Grammar #1527

driesvints opened this issue Feb 19, 2019 · 9 comments

Comments

@driesvints
Copy link
Member

driesvints commented Feb 19, 2019

At the moment a lot of schema grammar's have certain implementation details which are a bit odd. For example, the MySQL grammar's float implementation points to a double which is a bit unwanted (see laravel/framework#3151). Judging from a first look at the other grammar's there are quite a few other ones which are a bit off.

We should take a thorough look at each grammar and try to pinpoint every obscure implementation and attempt to replace and fix it with its correct one if possible.

https://github.com/laravel/framework/tree/5.8/src/Illuminate/Database/Schema/Grammars

We have to be very careful here since changing any implementation here will affect every single app using the affected methods.

I'll try to update the issue description now and then below with the grammar's we've determined to be off. After we've pinpointed all of them we maybe can send in a PR to fix them.

Since at the moment my time is limited to work on this I'd appreciate any help with this if possible.

References:

@driesvints
Copy link
Member Author

@staudenmeir If you have time for this, I'd really appreciate your opinion and insights :)

@mfn
Copy link

mfn commented Feb 19, 2019

As a tangent remark: one part of the confusion sometimes is also the reliance on \Fluent. A untyped bucket of any data you want to put it in it and each driver freely takes out what it understands or simply ignores it. The best "protection" currently are the phpdoc autocompletes for the IDE.

@staudenmeir
Copy link

staudenmeir commented Feb 20, 2019

@mfn It would be good to improve that, but I'm not sure how feasible it is. The individual grammars do have lists of supported modifiers, maybe we can use them. We could also consider custom classes like MySqlColumn.

@staudenmeir
Copy link

staudenmeir commented Feb 20, 2019

MySQL:

PostgreSQL:

  • typeFloat() is an alias for typeDouble(), but the documentation differentiates between real and double depending on the precision.

Code improvements:

  • Grammar should have default $modifiers and $serials properties.
  • We can remove the if(method_exists()) check in Grammar::addModifiers().
  • We could move shared type definitions like typeInteger() or typeText() from the individual grammars up to Grammar. Maybe every type should have a default definition in Grammar.
  • We should order the modifiers consistently.

@SjorsO
Copy link

SjorsO commented Feb 20, 2019

I agree with @mfn, it would be nice if the column definitions didn't rely on Fluent. The main problem is that currently, invalid method calls (such as a mistyped ->nullable()) call are simply ignored, giving the user no feedback that something went wrong. I've also recently worked on a legacy project that had a bunch of foreign key definition like this: $table->string('company_id')->references('id')->on('companies');, this doesn't work, but since no exceptions are thrown an inexperienced Laravel developer can wrongly assume that it works.

@driesvints
Copy link
Member Author

I'm all up for improving the mechanics of the Grammar implementation but please note that in the first place this issue is for using correct types in the grammer column methods. We need to track down which ones need to be changed.

@staudenmeir
Copy link

@driesvints I think we should try to get this into 6.0. What other issues did you find?

@driesvints
Copy link
Member Author

@staudenmeir I have to say my mind is a little bit vague about it since it's been a few months already.. Not sure if there have been more issues about this. I'm all up for improving this on the next release but probably won't have time myself.

@mfn
Copy link

mfn commented Jul 31, 2019

I just stumbled over https://github.com/PeeHaa/migres :

  • pgsql specific
  • but strongly typed, no Fluent, no ambiguities
  • it shows how much resources this requires: it "looks good" but still is alpha / not recommended for production; and this package is written by someone being totally dedicated to this
  • it's PHP 7.4 so a non-started for some time
  • nevertheless quite some admirable efforts there

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants