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

Fix an issue with default CURRENT TIMESTAMP value #178

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

JoshuaLuckers
Copy link
Contributor

When generating a schema based on the INFORMATION_SCHEMA.COLUMNS table, a default CURRENT TIMESTAMP is displayed as CURRENT_TIMESTAMP up until MariaDB 10.2.2, and as current_timestamp() from MariaDB 10.2.3, due to to MariaDB 10.2 accepting expressions in the DEFAULT clause.

Related issue: #130

…r newer

When generating a schema based on the INFORMATION_SCHEMA.COLUMNS table, a default CURRENT TIMESTAMP is displayed as CURRENT_TIMESTAMP up until MariaDB 10.2.2, and as current_timestamp() from MariaDB 10.2.3, due to to MariaDB 10.2 accepting expressions in the DEFAULT clause.
@cla-bot cla-bot bot added the cla-signed CLA confirmed for all contributors to this PR label May 26, 2020
@opengeek
Copy link
Member

I'm wondering if this wouldn't be better solved by calling strtoupper() on the value being compared to the list of _currentTimestamp values in the array. e.g.

in_array(strtoupper($v), $this->xpdo->driver->_currentTimestamps)

@JoshuaLuckers
Copy link
Contributor Author

For this particular case I wouldn't do that. Doing it this way makes it easier to figure out what's happening and why (if you check the git history for this line). Also I don't think it's worth the little overhead of strtoupper.

@opengeek opengeek merged commit 8b1daea into modxcms:2.x Jul 13, 2020
@JoshuaLuckers JoshuaLuckers deleted the 2x-issue-130 branch July 14, 2020 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for all contributors to this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants