Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option for addTimeStamps() to use datetime #2273

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Option for addTimeStamps() to use datetime #2273

merged 3 commits into from
Jun 3, 2024

Conversation

joshbmarshall
Copy link
Contributor

Adds a feature flag to use datetime on MySQL for addTimestamps() function instead of timestamp.

Implements #1635

timestamp columns do not support dates past 2038-01-19 on MySQL

@dereuromark
Copy link
Member

@cakephp/phinx Any reviews?

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@dereuromark
Copy link
Member

Will this be ported to migrations? I wonder if we should make a list of prs to migrate etc

@markstory
Copy link
Member

Will this be ported to migrations? I wonder if we should make a list of prs to migrate etc

In a way yes, I'm hoping to update the internals of migrations to use cake's db reflection so we'll inherit the fix made in cake. I can also port this change as is over it should apply.

markstory added a commit to cakephp/migrations that referenced this pull request Jun 3, 2024
Port changes from cakephp/phinx#2273 to the migrations backend. This
helps maintain compatibility with phinx.
Copy link
Member

@MasterOdin MasterOdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably enable this by default and it wouldn't have the largest blast radius, given that not sure how often people are relying on these auto-generated columns having a specific type. We could then add also getting timestamps or datetimes as an option to addTimestamps to help the transition for people who were relying on the timestamp type.

@joshbmarshall
Copy link
Contributor Author

joshbmarshall commented Jun 3, 2024

I had considered requesting to make it the default since timestamp fields are deprecated in MySQL. But didn't want to add delays to getting the option into the code so I can start using it ;)

It should definitely become the default on the next major revision of phinx if it has to wait until then.

@MasterOdin MasterOdin merged commit 1302418 into cakephp:0.x Jun 3, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants