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

Type time #12471

Closed
wants to merge 95 commits into
base: 4.0.x
from

Conversation

@ruudboon
Copy link
Member

ruudboon commented Dec 13, 2016

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?

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Dec 13, 2016

@sergeyklay sergeyklay added this to the 3.1.0 milestone Dec 13, 2016

ruudboon added some commits Dec 13, 2016

/**
* Special type for time
*/
let definition["type"] = Column::TYPE_TIME;

This comment has been minimized.

@sergeyklay

sergeyklay Dec 13, 2016

Member

Could you please fix indents

}elseif memstr(columnType, "time") {
/**
* Time
*/

This comment has been minimized.

@sergeyklay

sergeyklay Dec 13, 2016

Member

and here

This comment has been minimized.

@ruudboon

ruudboon Dec 13, 2016

Author Member

Sorry, missed that. Will fix it. Still working on those unit test btw. Try to push them today.

ruudboon added some commits Dec 13, 2016

if empty columnSql {
let columnSql .= "TIME";
}
break;

This comment has been minimized.

@sergeyklay

sergeyklay Dec 13, 2016

Member

there still issue with indents

if empty columnSql {
let columnSql .= "TIMESTAMP";
}
break;

This comment has been minimized.

@sergeyklay

sergeyklay Dec 13, 2016

Member

and here

@sergeyklay

This comment has been minimized.

Copy link
Member

sergeyklay commented Dec 13, 2016

Could you provide a solution for the SQLite. I mean converting TIME to date type, etc

@ruudboon

This comment has been minimized.

Copy link
Member Author

ruudboon commented Dec 14, 2016

I think everything is in. If there's more needed please let me know.

fix indents
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";

This comment has been minimized.

@sergeyklay

sergeyklay Dec 14, 2016

Member

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');

This comment has been minimized.

@sergeyklay

sergeyklay Dec 14, 2016

Member

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?

This comment has been minimized.

@ruudboon

ruudboon Dec 14, 2016

Author Member

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.

This comment has been minimized.

@Jurigag

Jurigag Jan 12, 2017

Member

There is already TYPE_DATETIME and TYPE_DATE

$column = new Column("time-test", array(
'type' => Column::TYPE_TIME
));
$this->assertEquals($dialect->getColumnDefinition($column), 'TIME');

This comment has been minimized.

@sergeyklay

sergeyklay Dec 14, 2016

Member

See my comment for MysqlTest

$column = new Column("time-test", array(
'type' => Column::TYPE_TIME
));
$this->assertEquals($dialect->getColumnDefinition($column), 'TIME');

This comment has been minimized.

@sergeyklay

sergeyklay Dec 14, 2016

Member

See my comment for MysqlTest

@david-duncan

This comment has been minimized.

Copy link

david-duncan commented Feb 22, 2017

This hasn't seen activity in two months - can we get review fixes pushed and for this PR to get squashed?

@sergeyklay sergeyklay modified the milestones: 3.1.0, 3.2.0 Mar 2, 2017

@sergeyklay sergeyklay force-pushed the phalcon:3.1.x branch from 928502b to b0a7493 Mar 9, 2017

@sergeyklay sergeyklay force-pushed the phalcon:3.1.x branch from f115731 to c7de98d Mar 21, 2017

@sergeyklay sergeyklay force-pushed the phalcon:3.1.x branch 4 times, most recently from a74b0f4 to 5489228 Apr 2, 2017

sergeyklay and others added some commits Oct 12, 2017

Don't send dispatcher parameters
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.
Merge pull request #13123 from dugwood/patch-1
Don't send dispatcher parameters
Merge upstream
Merge branch 'master' into type_time
@ruudboon

This comment has been minimized.

Copy link
Member Author

ruudboon commented Nov 8, 2017

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.

@niden niden self-assigned this Oct 22, 2018

@niden niden referenced this pull request Oct 31, 2018

Merged

[#13543] Add more pdo types #13562

3 of 3 tasks complete
@niden

This comment has been minimized.

Copy link
Member

niden commented Oct 31, 2018

Addressed in #13562

@niden niden closed this Oct 31, 2018

@niden niden referenced this pull request Oct 31, 2018

Open

Update 4.x Documents #1935

0 of 10 tasks complete

@niden niden added this to Done in 4.0 Release Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment