Skip to content
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

Add DateInterval type parameter #6734

Merged
merged 1 commit into from
Oct 11, 2017
Merged

Conversation

ossinkine
Copy link
Contributor

This PR adds the ability to pass a DateInterval object as a query parameter

@Ocramius
Copy link
Member

Looks good to me.

We need to plan something smarter for ORM v3 (maybe a replaceable infer mechanism?

@ossinkine
Copy link
Contributor Author

Tests with the lowest dependencies didn't pass: https://travis-ci.org/doctrine/doctrine2/jobs/280750301
Should I increase the version in composer.json or add the version check in the class?

@Ocramius
Copy link
Member

Ocramius commented Sep 28, 2017 via email

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for the CS fixes as well 😄

@lcobucci lcobucci self-assigned this Oct 11, 2017
@lcobucci lcobucci added this to the 2.6.0 milestone Oct 11, 2017
[[2], Connection::PARAM_INT_ARRAY],
[["foo"], Connection::PARAM_STR_ARRAY],
[["1","2"], Connection::PARAM_STR_ARRAY],
[[], Connection::PARAM_STR_ARRAY],
[true, Type::BOOLEAN],
];
];

if (PHP_VERSION_ID >= 50500) {
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but we can simplify this since we require PHP 7.1+

@lcobucci lcobucci merged commit 8e16748 into doctrine:master Oct 11, 2017
@lcobucci
Copy link
Member

@ossinkine 🚢

@@ -17,7 +17,7 @@
"php": "^7.1",
"ext-pdo": "*",
"doctrine/collections": "^1.4",
"doctrine/dbal": ">=2.5-dev,<2.7-dev",
"doctrine/dbal": "^2.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we allow >=2.7-dev intentionally here or is it just accidental?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it up intentionally for look at tests and they were passed, so I think it's ok.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was accidental =) It should indeed be >=2.6-dev,<2.7-dev or even >=2.6-dev,<2.8-dev

Copy link
Member

Choose a reason for hiding this comment

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

With how carefully we're checking BC, does it even matter? Might as well use the semver operator here.

Copy link
Member

Choose a reason for hiding this comment

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

That's true... @Majkl578 @Ocramius anything against that?

@Ocramius
Copy link
Member

Ocramius commented Oct 13, 2017 via email

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