barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.17k stars 1.16k forks source link

Nullable timestamps #1551

Closed juliangums closed 4 months ago

juliangums commented 4 months ago

Question:

Currently, timestamps can be null: * @property Carbon|null $created_at, * @property Carbon|null $updated_at. In reality, timestamps are never null though. It is just that it's been done to avoid some weird MySQL behaviour. All explained in this blog post: https://joelclermont.com/post/2020-11/why-are-eloquent-timestamps-nullable/

However, it seems silly for me to add null checks in the code everywhere if in reality it is never null. Is this something you'd consider changing? Or maybe check in migrations if someone is using the nullableTimestamps() method make them Carbon|null and if someone uses timestamps() make them Carbon only?

mfn commented 4 months ago

weird MySQL behaviour

Ack, other databases don't have this, which are supported by ide-helper too. E.g. pgsql does not have this issue; so any change has to be considered carefully.

For one, using pgsql, I stopped using these migrations in my code base where I could and replaced them with more explicit, not null, types (basically timestampTz())

Or maybe check in migrations

That would a "big thing" IMHO, because ide-helper is not concerned with migrations. It purely works on the actual database.


IMHO it feels wrong to not have the field reflect the nullable, when in fact the database colum is nullable.

Can you use the hooks feature in ide-helper to change this for your project?

juliangums commented 4 months ago

IMHO it feels wrong to not have the field reflect the nullable, when in fact the database colum is nullable.

I'd usually agree, but this case is very different. Given that 99.9999% of projects will not null those timestamps, I think the default shouldn't be for the vast majority to null-check them. Maybe this could be a config setting for those few that want nullable timestamps?

juliangums commented 4 months ago

I added a hook for now but would be happy to see this. I can see if I can help with a PR if it's wanted.

mfn commented 4 months ago

Can only try to share my point of view once more:

I simply think the additional complexity for this feature doesn't need to be in ide-helper 🤷🏼

juliangums commented 4 months ago

I get your point. Didn't take me long to create a hook for it. Just thought others could benefit from it, given that the vast majority of users will have the same setup.