Skip to content

[Yaml] set arguments depending on the PHP version #20408

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

Merged
merged 1 commit into from
Nov 4, 2016

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 4, 2016

Q A
Branch? 3.1
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

@nicolas-grekas
Copy link
Member

So, regarding #20291 (comment)
you propose: wait and see?

@xabbuh
Copy link
Member Author

xabbuh commented Nov 4, 2016

Yeah, I think it doesn't hurt to wait what HHVM will do in the future. We can still adapt the test then if needed.

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

if (PHP_VERSION_ID >= 70100) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work for different HHVM versions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on which PHP version they claim to be compatible with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got recommended to do a feature check where possible instead of a version check. Not sure if this is actually possible without reflection

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a test case, we don't care...

@fabpot
Copy link
Member

fabpot commented Nov 4, 2016

Thank you @xabbuh.

@fabpot fabpot merged commit ad54d83 into symfony:3.1 Nov 4, 2016
fabpot added a commit that referenced this pull request Nov 4, 2016
This PR was merged into the 3.1 branch.

Discussion
----------

[Yaml] set arguments depending on the PHP version

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

ad54d83 [Yaml] set arguments depending on the PHP version
@xabbuh xabbuh deleted the pr-20291 branch November 4, 2016 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants