ensure data is stored as UTC and returned in the default TZ #109

Merged
merged 1 commit into from Jun 1, 2013

Conversation

Projects
None yet
2 participants
@lsmith77
Member

lsmith77 commented Apr 25, 2013

No description provided.

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Apr 25, 2013

Member

there is still the fatal error on travis with calling setTimezone on null

Member

dbu commented Apr 25, 2013

there is still the fatal error on travis with calling setTimezone on null

break;
case PropertyType::DATE:
$date = $valueNode->nodeValue;
+ if ($date && !is_numeric($date)) {

This comment has been minimized.

@lsmith77

lsmith77 May 6, 2013

Member

when running the test suite we sometimes get numeric values for example:


195) PHPCR\Tests\NodeTypeManagement\NodeTypeTest::testPrimaryItem
Exception: DateTime::__construct(): Failed to parse time string (1) at position 0 (1): Unexpected character

/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:718
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:880
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:928
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/ObjectManager.php:279
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Node.php:597
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/inc/BaseCase.php:192
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/inc/BaseCase.php:159
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/tests/19_NodeTypeManagement/NodeTypeBaseCase.php:30

not sure why this is the case and if this is correct or not ..

@lsmith77

lsmith77 May 6, 2013

Member

when running the test suite we sometimes get numeric values for example:


195) PHPCR\Tests\NodeTypeManagement\NodeTypeTest::testPrimaryItem
Exception: DateTime::__construct(): Failed to parse time string (1) at position 0 (1): Unexpected character

/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:718
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:880
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/src/Jackalope/Transport/DoctrineDBAL/Client.php:928
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/ObjectManager.php:279
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/jackalope/jackalope/src/Jackalope/Node.php:597
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/inc/BaseCase.php:192
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/inc/BaseCase.php:159
/Users/lsmith/htdocs/cmf-sandbox/vendor/jackalope/jackalope-doctrine-dbal/vendor/phpcr/phpcr-api-tests/tests/19_NodeTypeManagement/NodeTypeBaseCase.php:30

not sure why this is the case and if this is correct or not ..

This comment has been minimized.

@lsmith77

lsmith77 May 6, 2013

Member

@dbu can you comment here?

@lsmith77

lsmith77 May 6, 2013

Member

@dbu can you comment here?

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 May 6, 2013

Member

@dbu: the failures we have now are mostly about comparing strings instead of timestamps in the test suite

Member

lsmith77 commented May 6, 2013

@dbu: the failures we have now are mostly about comparing strings instead of timestamps in the test suite

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 May 7, 2013

Member

@dbu can you provide some feedback here?

Member

lsmith77 commented May 7, 2013

@dbu can you provide some feedback here?

@ghost ghost assigned lsmith77 May 13, 2013

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 May 23, 2013

Member

first steps to refactor the unit tests phpcr/phpcr-api-tests#103

Member

lsmith77 commented May 23, 2013

first steps to refactor the unit tests phpcr/phpcr-api-tests#103

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu May 24, 2013

Member

this looks correct to me, i think fixing the test suite to not make assumptions about timezone makes sense. i gave some input on the test suite PR

Member

dbu commented May 24, 2013

this looks correct to me, i think fixing the test suite to not make assumptions about timezone makes sense. i gave some input on the test suite PR

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 May 28, 2013

Member

this is now ready for final review

requires phpcr/phpcr-api-tests#103

Member

lsmith77 commented May 28, 2013

this is now ready for final review

requires phpcr/phpcr-api-tests#103

+ if ($date) {
+ $date = new \DateTime($date);
+ $date->setTimezone(new \DateTimeZone(date_default_timezone_get()));
+ $date = $valueConverter->convertType($date, PropertyType::STRING);

This comment has been minimized.

@dbu

dbu May 29, 2013

Member

this step should not be needed. the Node constructor is passing everything through the value converter to be sure but that will just return early when it sees that a date already is a date. if you tried and it did not work, i propose to add a comment to this line why its needed, if we can't fix the underlying issue easily.

@dbu

dbu May 29, 2013

Member

this step should not be needed. the Node constructor is passing everything through the value converter to be sure but that will just return early when it sees that a date already is a date. if you tried and it did not work, i propose to add a comment to this line why its needed, if we can't fix the underlying issue easily.

This comment has been minimized.

@lsmith77

lsmith77 May 29, 2013

Member

we need to convert it to a string date here ..

@lsmith77

lsmith77 May 29, 2013

Member

we need to convert it to a string date here ..

This comment has been minimized.

@dbu

dbu May 29, 2013

Member

Can you please add a comment to the code why we need string? There is a todo in node about the stdclass vs something more convenient.

@dbu

dbu May 29, 2013

Member

Can you please add a comment to the code why we need string? There is a todo in node about the stdclass vs something more convenient.

+ $date->setTimezone(new \DateTimeZone('UTC'));
+ } elseif (PropertyType::DATE !== $this->valueConverter->determineType($date)) {
+ throw new ValueFormatException("Value '$date' is not a date but a '".PropertyType::nameFromValue($this->valueConverter->determineType($date))."'");
+ }

This comment has been minimized.

@dbu

dbu May 29, 2013

Member

i think doing $property->getDate() should guarantee the value is of type \DateTime, so we could just do a sanity check and throw some exception if its not of that type. i would follow the same model as elsewhere here and box the single date into an array if the property isMultiple is false, then loop over the array to set the timezone. not sure if even the sanity check is needed, Property->getDate is part of jackalope and tested.

@dbu

dbu May 29, 2013

Member

i think doing $property->getDate() should guarantee the value is of type \DateTime, so we could just do a sanity check and throw some exception if its not of that type. i would follow the same model as elsewhere here and box the single date into an array if the property isMultiple is false, then loop over the array to set the timezone. not sure if even the sanity check is needed, Property->getDate is part of jackalope and tested.

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu May 29, 2013

Member

i already merged the phpcr-api-tests cleanup. can you check my two comments and see if we can simplify the code further? on the next commit travis should no longer fail i hope.

Member

dbu commented May 29, 2013

i already merged the phpcr-api-tests cleanup. can you check my two comments and see if we can simplify the code further? on the next commit travis should no longer fail i hope.

@lsmith77

This comment has been minimized.

Show comment
Hide comment
@lsmith77

lsmith77 May 29, 2013

Member

ok fixed .. we will also need to think about a migration script here. i guess all that is needed is a script that "touches" each node with a date to ensure that its resaved. that being said, everything should work without changes .. ie. the "touching" is only needed to get the new consistent date behavior.

Member

lsmith77 commented May 29, 2013

ok fixed .. we will also need to think about a migration script here. i guess all that is needed is a script that "touches" each node with a date to ensure that its resaved. that being said, everything should work without changes .. ie. the "touching" is only needed to get the new consistent date behavior.

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Jun 1, 2013

Member

i think this looks right, so i would merge it. it won't break anything or make things worse with existing installations so the migration is not needed for the merge.

Member

dbu commented Jun 1, 2013

i think this looks right, so i would merge it. it won't break anything or make things worse with existing installations so the migration is not needed for the merge.

lsmith77 added a commit that referenced this pull request Jun 1, 2013

Merge pull request #109 from jackalope/utc
ensure data is stored as UTC and returned in the default TZ

@lsmith77 lsmith77 merged commit 1855d18 into master Jun 1, 2013

1 check passed

default The Travis CI build passed
Details

@lsmith77 lsmith77 deleted the utc branch Jun 7, 2013

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