[YAML] Fix processing timestamp strings with timezone #20550

noahheck · 2016-11-17T15:16:17Z

Q	A
Branch?	3.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20399
License	MIT
Doc PR	-

Parse date strings containing timezone data correctly
Default date strings not containing timezone data to UTC

fabpot · 2016-11-17T15:48:37Z

src/Symfony/Component/Yaml/Inline.php

+                            // Timestamps that have timezone data should be parsed in the specified timezone first, then
+                            // converted to UTC; if no tz data is supplied, parse as UTC date initially
+                            if (isset($datetimeMatches['tz'])) {
+                                $date = new \DateTime($scalar);


missing $datetimeMatches['tz'] here?

The timezone data is in the string ($scalar) so no need to add it to the constructor.

stof · 2016-11-17T15:52:31Z

Tests are needed to cover this

noahheck · 2016-11-17T16:19:24Z

@stof There are tests for this, but they were failing in non-UTC timezone: #20551

xabbuh · 2016-11-17T16:41:04Z

#20551 should make such bugs visible in the future. So yes, I agree that we don't have to update the tests.

ro0NL · 2016-11-17T18:22:31Z

@nicolas-grekas why did you removed BC break label? :)

ro0NL · 2016-11-17T18:24:11Z

src/Symfony/Component/Yaml/Tests/InlineTest.php

@@ -535,7 +535,11 @@ public function testParseNestedTimestampListAsDateTimeObject($yaml, $year, $mont
        $expected = new \DateTime($yaml);
        $expected->setTimeZone(new \DateTimeZone('UTC'));
        $expected->setDate($year, $month, $day);
-        @$expected->setTime($hour, $minute, $second, 1000000 * ($second - (int) $second));


I believe this was intended to cover HHVM. Not sure we should remove it.

This patch is just adding consistency with another test in the same file. Looks like we missed it, maybe during a merge.

Found it :) #20291 (comment)

Ah.. i see the inconsistency now 👍

nicolas-grekas · 2016-11-17T18:27:04Z

@ro0NL why should we keep it in the first place?

ro0NL · 2016-11-17T18:29:31Z

Keep what? The spec is correct now, but the BC break is real right? Before system timezone was the default, now UTC 😕

noahheck · 2016-11-17T23:22:25Z

@ro0NL All parsed DateTimes were converted to UTC. Before, the system timezone was used only used if there wasn't timezone information in the timestamp string. The DateTime was then converted to UTC time, but converting had the possibility of changing the representation. The returned DateTime will still be in UTC, just without the value being changed.

I don't know if that constitutes a BC break (?)

ro0NL · 2016-11-18T08:47:17Z

Before datetimes without a timezone assumed the system timezone, before UTC conversion. Now UTC is assumed.. (ie. no conversion happens).

What about this approach?

// in 4.0
// return new \DateTime($scalar, new \DateTimeZone('UTC'));

if ('UTC' !== date_default_timezone_get()) {
   // trigger deprecation: datetimes without a timezone are assumed UTC in 4.0 instead of the system timezone
}

// current code, ie.
$date = new \DateTime($scalar);
$date->setTimeZone(new \DateTimeZone('UTC'));

return $date;

nicolas-grekas · 2016-11-18T14:55:15Z

@ro0NL a deprecation is something that a user must be able to resolve by changing its code. Not the case here. This is clearly a bug to me. Every bug fix is a behavior change by definition.

ro0NL · 2016-11-18T15:06:55Z

You can change the datetime value, so it includes the timezone explicitly. Which is only needed, if the system timezone is not UTC already. This gives correct YAML, as well as the expected value.

Just saying this can lead to tricky side effects, as we get a total different time value.

nicolas-grekas · 2016-11-18T15:18:38Z

Changing a date definition in yaml to workaround a parser implementation bug is definitely not a "code fix"...

ro0NL · 2016-11-18T15:24:05Z

src/Symfony/Component/Yaml/Inline.php

                        if (Yaml::PARSE_DATETIME & $flags) {
-                            $date = new \DateTime($scalar);


Given @nicolas-grekas comment, it could simply be

$date = new \DateTime($scalar, new \DateTimeZone('UTC')); $date->setTimeZone(new \DateTimeZone('UTC')); return $date;

stof · 2016-11-18T16:20:11Z

If the Yaml timestamp does not include the timezone info, parsing it as a UTC timestamp is the right thing to do according the behavior of the official PyYaml parser.
So 👍 for this

ro0NL · 2016-11-18T17:30:02Z

Would a changelog help? To clarify, i know configs defining a datetime value (some specific 'since' date that is updated once in a while) which will now silently fetch data from one hour earlier than expected (system tz = +01:00).

People should be aware of this change, to update accordingly.

nicolas-grekas · 2016-11-18T20:40:48Z

👍

This PR was merged into the 2.7 branch. Discussion ---------- [ci] Testing with UTC hides bugs | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As shown by #20550, we're having bugs because we don't in proper TZ conditions. Let's do Europe on Travis, and America on Windows. Commits ------- e0f9bda [ci] Testing with UTC hides bugs

nicolas-grekas · 2016-11-18T20:52:15Z

@ro0NL this will be in the usual changelog and github history.

ro0NL · 2016-11-18T20:59:27Z

Correct.. i meant a upgrade note actually. But i guess this is fine then, lets see what happens :)

👍

Parse date strings containing timezone data correctly Default date strings not containing timezone data to UTC

nicolas-grekas · 2016-11-18T21:07:15Z

In fact, this bug comes from #20291 which is quite recent.
I reverted the UTC normalization behavior that I wrongly introduced then, and added test cases for the correct behavior (preserving TZ info).

nicolas-grekas · 2016-11-18T21:17:33Z

Thank you @noahheck.

…sain) This PR was merged into the 3.1 branch. Discussion ---------- [YAML] Fix processing timestamp strings with timezone | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20399 | License | MIT | Doc PR | - Parse date strings containing timezone data correctly Default date strings not containing timezone data to UTC Commits ------- cdb11a7 [YAML] Fix processing timestamp with timezone

NicoHaase · 2016-11-21T10:17:29Z

In Travis, you can run test builds using a configuration matrix. I think, such a feature could help to run these tests more thoroughly. Running with configuration A on one test server and B on another introduces new ways of bugs. Running with configuration A AND B on one server and the same configurations on another server sounds much better

//edit: sorry, this should go to e968d0f

nicolas-grekas · 2016-11-21T10:45:57Z

We care a lot about the total time it takes for our CI to complete. Doing this wouldn't provide any more safety to me, but make tests run slower.

NicoHaase · 2016-11-21T11:35:27Z

Travis builds should run parallel, so you start one run using one timezone setting and another run using the different setting at the same time. It should not take noticable more time to run tests with multiple settings.

In my opinion: if there is the possibility that Symfony behaves differently for different settings, all such combinations should be tested to ensure that Symfony behaves correctly in all these settings. Skipping combinations to fasten up the test suite sounds like the wrong way

nicolas-grekas · 2016-11-21T11:59:00Z

We're not skipping combinations, we're just splitting them to several ci providers.
And even if tests run in // (which is already the case right now of course), since the number of jobs is limited, adding more jobs would still slow down the ci experience when people submit several PRs concurrently, which happens a lot on an active project like Sf.

carsonbot added Status: Needs Review Yaml Bug Feature BC Break Deprecation labels Nov 17, 2016

noahheck force-pushed the ticket_20399 branch from 92e5b6d to bf109b1 Compare November 17, 2016 15:22

noahheck changed the base branch from master to 3.1 November 17, 2016 15:24

nicolas-grekas removed BC Break Deprecation Feature labels Nov 17, 2016

fabpot reviewed Nov 17, 2016

View reviewed changes

nicolas-grekas mentioned this pull request Nov 17, 2016

[ci] Testing with UTC hides bugs #20551

Merged

noahheck force-pushed the ticket_20399 branch from bf109b1 to 1d050d5 Compare November 17, 2016 16:15

ro0NL reviewed Nov 17, 2016

View reviewed changes

ro0NL reviewed Nov 18, 2016

View reviewed changes

nicolas-grekas force-pushed the ticket_20399 branch from 1d050d5 to ae93c96 Compare November 18, 2016 20:37

nicolas-grekas force-pushed the ticket_20399 branch from ae93c96 to 1b5b04a Compare November 18, 2016 20:39

[YAML] Fix processing timestamp with timezone

cdb11a7

Parse date strings containing timezone data correctly Default date strings not containing timezone data to UTC

nicolas-grekas force-pushed the ticket_20399 branch from 1b5b04a to cdb11a7 Compare November 18, 2016 21:05

nicolas-grekas merged commit cdb11a7 into symfony:3.1 Nov 18, 2016

fabpot mentioned this pull request Nov 21, 2016

Release v3.1.7 #20575

Merged

fabpot mentioned this pull request Nov 27, 2016

Release v3.2.0-RC2 #20649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YAML] Fix processing timestamp strings with timezone #20550

[YAML] Fix processing timestamp strings with timezone #20550

noahheck commented Nov 17, 2016 •

edited

Loading

fabpot Nov 17, 2016

noahheck Nov 17, 2016

stof commented Nov 17, 2016

noahheck commented Nov 17, 2016

xabbuh commented Nov 17, 2016 •

edited

Loading

ro0NL commented Nov 17, 2016

ro0NL Nov 17, 2016

nicolas-grekas Nov 17, 2016

ro0NL Nov 17, 2016 •

edited

Loading

nicolas-grekas commented Nov 17, 2016

ro0NL commented Nov 17, 2016

noahheck commented Nov 17, 2016

ro0NL commented Nov 18, 2016 •

edited

Loading

nicolas-grekas commented Nov 18, 2016 •

edited

Loading

ro0NL commented Nov 18, 2016 •

edited

Loading

nicolas-grekas commented Nov 18, 2016 •

edited

Loading

ro0NL Nov 18, 2016

nicolas-grekas Nov 18, 2016

stof commented Nov 18, 2016

ro0NL commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

ro0NL commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

NicoHaase commented Nov 21, 2016 •

edited

Loading

nicolas-grekas commented Nov 21, 2016

NicoHaase commented Nov 21, 2016

nicolas-grekas commented Nov 21, 2016 •

edited

Loading

		if (Yaml::PARSE_DATETIME & $flags) {
		$date = new \DateTime($scalar);

[YAML] Fix processing timestamp strings with timezone #20550

[YAML] Fix processing timestamp strings with timezone #20550

Conversation

noahheck commented Nov 17, 2016 • edited Loading

fabpot Nov 17, 2016

Choose a reason for hiding this comment

noahheck Nov 17, 2016

Choose a reason for hiding this comment

stof commented Nov 17, 2016

noahheck commented Nov 17, 2016

xabbuh commented Nov 17, 2016 • edited Loading

ro0NL commented Nov 17, 2016

ro0NL Nov 17, 2016

Choose a reason for hiding this comment

nicolas-grekas Nov 17, 2016

Choose a reason for hiding this comment

ro0NL Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Nov 17, 2016

ro0NL commented Nov 17, 2016

noahheck commented Nov 17, 2016

ro0NL commented Nov 18, 2016 • edited Loading

nicolas-grekas commented Nov 18, 2016 • edited Loading

ro0NL commented Nov 18, 2016 • edited Loading

nicolas-grekas commented Nov 18, 2016 • edited Loading

ro0NL Nov 18, 2016

Choose a reason for hiding this comment

nicolas-grekas Nov 18, 2016

Choose a reason for hiding this comment

stof commented Nov 18, 2016

ro0NL commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

ro0NL commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

nicolas-grekas commented Nov 18, 2016

NicoHaase commented Nov 21, 2016 • edited Loading

nicolas-grekas commented Nov 21, 2016

NicoHaase commented Nov 21, 2016

nicolas-grekas commented Nov 21, 2016 • edited Loading

noahheck commented Nov 17, 2016 •

edited

Loading

xabbuh commented Nov 17, 2016 •

edited

Loading

ro0NL Nov 17, 2016 •

edited

Loading

ro0NL commented Nov 18, 2016 •

edited

Loading

nicolas-grekas commented Nov 18, 2016 •

edited

Loading

ro0NL commented Nov 18, 2016 •

edited

Loading

nicolas-grekas commented Nov 18, 2016 •

edited

Loading

NicoHaase commented Nov 21, 2016 •

edited

Loading

nicolas-grekas commented Nov 21, 2016 •

edited

Loading