laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

[proposal] Somewhere in the codebase we should be able to see all the database fields #1273

Closed alimranahmed closed 6 years ago

alimranahmed commented 6 years ago

Hello everyone who care about Laravel,

Problem

Currently, Let's say we have an instance of User model, $user. Now, we want to know whether that user has a mobile field/property or not. From which portion of the codebase we will be able to know that the user has a mobile? Migration could be an answer. But there might be 10 migration file regarding user among hundreds of all migrations. So, it really tedious to know this simple and obvious information from the codebase which is really unfortunate.

Proposals

We can put all the fields in our fields in Model file. If those file updated we can generate the migration file, then it will generate migrations for us. Yeah, just like Django(Framework of Python)

OR

To keep things separate, we can make another set of classes/config files where we can put all the schema information. If we update the those files then we can just generate migration using a simple command then migrations will be generate automatically, those migrations will be just like the current migrations but auto generated from schema files.

Patryk27 commented 6 years ago

There already exists a property which is utilized in this fashion - $fillable.

Additionally, no one forbids you from doing:

/**
 * @property-read int $id
 * @property string $something
 * @property string $something_else
 * @property-read Carbon $created_at
 * @property-read Carbon $updated_at
 *
 * -----
 *
 * @property-read SomeRelatedModel $someRelatedModel
 */
class SomeModel extends Model 
{

  /* ... */

}

... and bam, you even get PhpStorm's autocompletion working this way.

mfn commented 6 years ago

https://github.com/barryvdh/laravel-ide-helper is able to introspect/generate phpdoc @property-… data on your models directly:

/**
 * App\Post
 *
 * @property int $id
 * @property int $some_id
 */
class Post {

Together with https://github.com/phpstan/phpstan you can automate discovering access to undefined attributes as long as you're diligent with your types declaration/hints.

alimranahmed commented 6 years ago

So if I change the migration file I need to update those property annotation also, which is repetition of of work(Anti DRY). Besides, if someone in the team forgets to update those comments(which is very likely) then this won't work. And as a responsible developer if you want to update those property which has not been updated someone else, you will face the same problem as mention in the proposal description.

Patryk27 commented 6 years ago

Anti DRY

Don't repeat yourself applies to situations where repetition of code makes it harder to maintain - this is not the case here, because adding those PHPDocs actually makes code easier to understand and easier to perform a static analysis on (that is: PhpStorm's autocompletion kicks in, for example).

if someone in the team forget to update those comments(which is very likely)

Why would you work on a serious application with people who do not understand a simple if you create a migration, you also have to update the model command?

We could as well say that immutable migrations are pointless, because it is very likely that some beginner will modify an old, already-existing migration.

I can tell you from my experience that people generally tend to remember about those PHPDocs, if you just tell them to do so.

if you want to update those property which has not been updated someone else, you will face the same problem as mention in the proposal description.

Sorry, I do not understand this one - could you elaborate a bit more?

mfn commented 6 years ago

Besides, if someone in the team forgets to update those comments(which is very likely) then this won't work

Yes, I understand you. It can be a source of errors. But mind you, you can also automate this in your CI/CD environment.

From a practical perspective: with the introduction of the ide helper many years ago and phpstan not so many years ago, this is the least of issues coming up on a regular base if at all on my team.

alimranahmed commented 6 years ago

@Patryk27

DRY

Yeah, I know it's about code. But as a developer we don't want to do the same thing two time. Already done it in migration then do it again in model. And yet it's in your codebase, not code though. Forgetting to update the model is very likely because of this same reason, once I have done it so if it's not a very regular habit it can be forgotten by a developer.

As @mfn said, CI/CD can be a helping hand to automate these staffs, though I don't know how to do that.

Clarification

Besides, if someone in the team forgets to update those comments(which is very likely) then this won't work

If someone somehow missed to update the model. Now as a responsible developers we want to fix that. What we need to do? Search all the migrations related users and update the User model accordingly.

Finally

Let me clarify my points again based on the responses. We don't have any reliable and single source of information in the code base from where we can tell that these are the fields of users or any other table. Someone may or may not update the annotation comment in Model. Even if someone don't update the comment the code will still work. So, these are not reliable though these provide the IDE a way to suggest us. Here, my motivation is just opposite(of who have respond so far). I am proposing a single source of information based on which the migration will be generated. If those information changes then the migration will be change automatically(my be by running a command). But current way is that migration will be written first and we need to syc(if it's manual then as a developer I am really convinced to follow) the Model based on migrations.

mfn commented 6 years ago

Summed up, everything you said has truth in it. I think what you are searching for is not part of the philosophy of how Eloquent models work.

I reads however rather as you would like to work with Doctrine ORM which AFAIK works/can work this way. I've also read you can use Doctrine with Laravel, but I've no experience with that.

I can only speak from experience that in a professional/well educated team over years this has never been a real issue.

As @mfn said, CI/CD can be a helping hand to automate these staffs, though I don't know how to do that.

You can run ide-helper as part of your CI and let it write/overwrite the models. If there's a diff (e.g. git diff…), then you know someone forgot to update the definitions and let the build fail.

sisve commented 6 years ago

As mentioned, Doctrine has you covered. Doctrine maps the database columns to model properties based on your mapping annotations. These can be either written as docblocks, xml files, or yaml files. This way there's a single source where you can find a list of all mapped database columns.

alimranahmed commented 6 years ago

@sisve & @mfn

Thanks for clarifications. I used Doctrine long time ago when I was an absolute beginner. Nice to know that Doctrine does it this way. But I am not sure about the migrations. What about them in Doctrine? My proposal was actually not only the fields of table in model but also their automatic sync(or may be with just a command) with the migrations. Is it covered in Doctrine or Symfony. I have already learnt from your reply that it is achievable even in Laravel. But in that case we need to add the fields in migrations and models both. Which is duplicate of work(or to be straight I am not convinced). Does Doctrine resolve this issue?

sisve commented 6 years ago

In the case of Doctrine there's a single source of truth, the mapping file. "The User model's email field is mapped to a 255 character long string, so say we all!" With that in mind the migration system will look at the current database structure, and can tell you what differs by generating the sql statements required to bring it up-to-date.

The migration system uses this functionality by creating timestamped migration files that contain these sql statements. You can find an example at https://laraveldoctrine.org/docs/1.3/migrations/diff