$timestamps = false is set as default on Token model

[posiunas]

I have created the index page for personal access tokens in my application and tried to output the time every record was created like this $token->created_at->diffForHumans() since I expect that created_at and updated_at should be mutated to Carbon instances like documented. But it requires $timestamps = true at first so those fields will be mutated:

https://github.com/laravel/framework/blob/16983a689d15333e7101457c3e0c0d0b7da01d69/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L803-L810

Token model with current $timestamps = false value:

passport/src/Token.php

Line 54 in 86af026

public $timestamps = false;

Migration for access_tokens table implies that table has timestamps:

passport/database/migrations/2016_06_01_000002_create_oauth_access_tokens_table.php

Line 23 in 3492cc0

$table->timestamps();

It is a very quick fix and I will make a PR easily but I am aware that maybe it has a purpose by setting $timestamps to false.

[rickmacgillis]

Re: #787 (@taylorotwell)

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

As this is a small change, I don't see a reason why the existing tests won't cover this change. However, the other questions seem useful to answer as I'm working with Laravel Nova and it requires the fields to be cast to 'datetime' to use them with the DateTime object. However, even though I could just use the $casts property to do it, it doesn't seem like a smart idea as admin changes won't get picked up by the fields for their intended purpose.

So, I looked up the commit history, and the first ever version of the Token class had the $timestamps turned off. So, I looked through the codebase and found that the fields are getting set manually to the current DateTime only when they get created. They never get changed ever again. (I did a file search on the src directory for created_at and updated_at to see that they don't get used anywhere else.)

That presumably was done to keep all tables synchronized on creation instead of a possible 1 second delay on a loaded server creating datetime discrepancies. However, the fact that Passport can still manually set those fields on creation keeps that concept intact, thus getting rid of any possible "breaking" changes.

I believe that answers Taylors original request.

Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

[driesvints]

@rickmacgillis hmm I'm actually not sure why it can't be enabled. I get that token records shouldn't update but at the same time it takes away functionality like @justinasposiunas described above. I'll mark this for review so @taylorotwell can have a look and perhaps tell us why this is done in this way.

[Rkallenkoot]

Perhaps the better way is to add created_at and updated_at to the $dates property.

This way it doesn't interfere with how the Tokens' timestamps are created and updated, but we can still use them as dates without having to create Carbon instances every time.

[driesvints]

PR was attempted here: #1075

I'll take a look at this somewhere this month.

[driesvints]

After looking into this again I agree with @rickmacgillis that this isn't necessary because the timestamps are indeed getting set manually. If you still want to customize this behavior you can override the model with https://laravel.com/docs/6.x/passport#overriding-default-models