Type time #12471
Type time #12471
Conversation
|
I would like a small test (see example here) using this namespace https://github.com/phalcon/cphalcon/tree/master/tests/unit/Db/Column |
phalcon/db/adapter/pdo/mysql.zep
Outdated
| /** | ||
| * Special type for time | ||
| */ | ||
| let definition["type"] = Column::TYPE_TIME; |
There was a problem hiding this comment.
Could you please fix indents
| }elseif memstr(columnType, "time") { | ||
| /** | ||
| * Time | ||
| */ |
There was a problem hiding this comment.
Sorry, missed that. Will fix it. Still working on those unit test btw. Try to push them today.
phalcon/db/dialect/mysql.zep
Outdated
| if empty columnSql { | ||
| let columnSql .= "TIME"; | ||
| } | ||
| break; |
There was a problem hiding this comment.
there still issue with indents
phalcon/db/dialect/postgresql.zep
Outdated
| if empty columnSql { | ||
| let columnSql .= "TIMESTAMP"; | ||
| } | ||
| break; |
|
Could you provide a solution for the SQLite. I mean converting TIME to date type, etc |
|
I think everything is in. If there's more needed please let me know. |
Hopefully all fixed now. Will try to hook up a normal editor on the vagrant machine. Sorry for all the hassle.
|
|
||
| case Column::TYPE_TIME: | ||
| if empty columnSql { | ||
| let columnSql .= "TIME"; |
There was a problem hiding this comment.
Are you sure about this? https://www.sqlite.org/datatype3.html#date_and_time_datatype
That's why I asked you to provide a workaround. That is why we are not in a hurry to accept all possible data types.
| $column = new Column("time-test", array( | ||
| 'type' => Column::TYPE_TIME | ||
| )); | ||
| $this->assertEquals($dialect->getColumnDefinition($column), 'TIME'); |
There was a problem hiding this comment.
I'd like to see here a test for the table that has this data type. Please understand me correctly. We can't release framework with black box.
For example:
CREATE TABLE `entries` (
`id` INT PRIMARY KEY AUTO_INCREMENT,
`start` TIME NOT NULL DEFAULT CURRENT_TIME,
`end` TIME DEFAULT '23:59',
`add` TIME NULL,
`amount` INT NOT NULL,
`price` INT NOT NULL
);Could you please convert this into objects and back?
There was a problem hiding this comment.
Hmm I get it. Makes sense! Shouldn't we have them for datetime etc also?
Thnx 4 all the feedback. Appreciate the effort, kinda new to this.
There was a problem hiding this comment.
There is already TYPE_DATETIME and TYPE_DATE
| $column = new Column("time-test", array( | ||
| 'type' => Column::TYPE_TIME | ||
| )); | ||
| $this->assertEquals($dialect->getColumnDefinition($column), 'TIME'); |
There was a problem hiding this comment.
See my comment for MysqlTest
| $column = new Column("time-test", array( | ||
| 'type' => Column::TYPE_TIME | ||
| )); | ||
| $this->assertEquals($dialect->getColumnDefinition($column), 'TIME'); |
There was a problem hiding this comment.
See my comment for MysqlTest
|
This hasn't seen activity in two months - can we get review fixes pushed and for this PR to get squashed? |
sergeyklay
force-pushed
the
3.1.x
branch
4 times, most recently
from
a74b0f4
to
5489228
Compare
These will erase previously set variables, and dispatcher's params were never accessible this way so far, so removing the argument in this function is fine.
Don't send dispatcher parameters
Merge branch 'master' into type_time
|
Just wanna give heads up that I did some work on this issue (ruudboon@ce2408d). Trying to finish unit tests and get Travis to work on my branch before submitting a new pull. |
|
Addressed in #13562 |
Rebased upon request #12293
@sergeyklay Looked at the other added Column types but didn't see any changed test. What test do you want me to change?