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.

Sign up for GitHub

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 support second, minute, week, year on DATE_ADD and DATE_SUB #6742

Merged

lcobucci merged 2 commits into doctrine:master from SebLevDev:feature/DDC-2938_Support_all_time_intervals_on_DATE_ADD

Oct 31, 2017

Contributor

SebLevDev commented Oct 2, 2017

The DATE_ADD and DATE_SUB built-in function of Doctrine supports only the "hour", "month" and "day" intervals. I don't see the point of not having all the intervals available. These should be "minute", "second" and "year". I didn't check compatibility with other database vendors rather than MySQL but I don't think it should be a problem

Cfr Also Ticket #3698

Ocramius suggested changes

View reviewed changes

Member

Ocramius left a comment

Overall, patch looks good, but misses testing (especially integration with other DBs

lib/Doctrine/ORM/Query/AST/Functions/DateAddFunction.php Outdated

@@ @@ -51,7 +51,11 @@ public function getSql(SqlWalker $sqlWalker) @@
  $this->firstDateExpression->dispatch($sqlWalker),
  $this->intervalExpression->dispatch($sqlWalker)
  );
+ case 'minute':

Member

Ocramius Oct 2, 2017

I spotted some \t here ;-)

Ocramius assigned SebLevDev

Ocramius added the Improvement label

Contributor Author

SebLevDev commented Oct 3, 2017

Can i do something again ?

Member

Ocramius commented Oct 3, 2017

Looks good to me: so far.

Assigning to @lcobucci for another pair of eyes.

Ocramius requested a review from lcobucci

October 3, 2017 12:43

Ocramius assigned lcobucci and unassigned SebLevDev

lcobucci suggested changes

View reviewed changes

Member

lcobucci left a comment

It LGTM, just some nitpicking. Also, aren't we missing tests for year and week add/sub?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ /**
+ * @group DDC-2938
+ */
+ public function testDateAddTime()

Member

lcobucci Oct 3, 2017

Please add : void

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ {
+ $arg = $this->_em->createQuery("SELECT DATE_ADD(CURRENT_TIMESTAMP(), 10, 'second') AS add FROM Doctrine\Tests\Models\Company\CompanyManager m")
+ ->getArrayResult();
+ $this->assertTrue(strtotime($arg[0]['add']) > 0);

Member

lcobucci Oct 3, 2017

Use self::assert*() instead

Member

lcobucci Oct 3, 2017

Wouldn't be better to use a more specific assertion?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ $arg = $this->_em->createQuery("SELECT DATE_ADD(CURRENT_TIMESTAMP(), 10, 'hour') AS add FROM Doctrine\Tests\Models\Company\CompanyManager m")
+ ->getArrayResult();
+ $this->assertTrue(strtotime($arg[0]['add']) > 0);

Member

lcobucci Oct 3, 2017

Use self::assert*() instead

Member

lcobucci Oct 3, 2017

Wouldn't be better to use a more specific assertion?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ $arg = $this->_em->createQuery("SELECT DATE_ADD(CURRENT_TIMESTAMP(), 10, 'minute') AS add FROM Doctrine\Tests\Models\Company\CompanyManager m")
+ ->getArrayResult();
+ $this->assertTrue(strtotime($arg[0]['add']) > 0);

Member

lcobucci Oct 3, 2017

Use self::assert*() instead

Member

lcobucci Oct 3, 2017

Wouldn't be better to use a more specific assertion?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ {
+ $arg = $this->_em->createQuery("SELECT DATE_SUB(CURRENT_TIMESTAMP(), 10, 'second') AS add FROM Doctrine\Tests\Models\Company\CompanyManager m")
+ ->getArrayResult();
+ $this->assertTrue(strtotime($arg[0]['add']) > 0);

Member

lcobucci Oct 3, 2017

Use self::assert*() instead

Member

lcobucci Oct 3, 2017

Wouldn't be better to use a more specific assertion?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ $arg = $this->_em->createQuery("SELECT DATE_SUB(CURRENT_TIMESTAMP(), 10, 'minute') AS add FROM Doctrine\Tests\Models\Company\CompanyManager m")
+ ->getArrayResult();
+ $this->assertTrue(strtotime($arg[0]['add']) > 0);

Member

lcobucci Oct 3, 2017

Use self::assert*() instead

Member

lcobucci Oct 3, 2017

Wouldn't be better to use a more specific assertion?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

+ $arg = $this->_em->createQuery("SELECT DATE_SUB(CURRENT_TIMESTAMP(), 10, 'hour') AS add FROM Doctrine\Tests\Models\Company\CompanyManager m")
+ ->getArrayResult();
+ $this->assertTrue(strtotime($arg[0]['add']) > 0);

Member

lcobucci Oct 3, 2017

Use self::assert*() instead

Member

lcobucci Oct 3, 2017

Wouldn't be better to use a more specific assertion?

tests/Doctrine/Tests/ORM/Functional/QueryDqlFunctionTest.php Outdated

@@ @@ -335,6 +353,24 @@ public function testDateSub() @@
  }
  /**
+ * @group DDC-2938
+ */
+ public function testDateSubTime()

Member

lcobucci Oct 3, 2017

Please add : void

Contributor Author

SebLevDev commented Oct 4, 2017

@lcobucci I cannot change for self::assert*() and : void .
These changes must be the subject of another PR.

Member

lcobucci commented Oct 4, 2017

@Legenyes you can change for the code you're inserting.

Contributor Author

SebLevDev commented Oct 4, 2017

It's worse.
Mixing two syntax in the same file is not a good practice.
I will not make this change in this PR

What about BC for : void ?

Member

Ocramius commented Oct 4, 2017 via email

This patch would land in 2.6 only, which is 7.1 only anyway

On 4 Oct 2017 12:08, "Sébastien Lévêque" ***@***.***> wrote: It's worse. Mixing two syntax in the same file is not a good practice. I will not make this change in this PR What about BC for : void ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakIsNIeuJfwNJ6M0wMq73_S22-w2Cks5so1kmgaJpZM4Pqbxg> .

Member

lcobucci commented Oct 4, 2017 •

edited

Loading

@Legenyes self::assert*() has already been fixed for v3 on the entire codebase, this means that new code should adhere to that, otherwise we need to reapply things over and over again. On the other hand, if we apply the change on v2 as well we'll be creating conflicts.

I agree that it's not ideal to have mixed syntaxes but it is the least worse solution, given this context.

Contributor Author

SebLevDev commented Oct 6, 2017

What' news ?

Member

lcobucci commented Oct 6, 2017

@Legenyes we still don't have tests for week and year and we still have the same self::assertTrue() checks that IMO are not really helpful since it doesn't provide you enough info about the operation that has been done.

Member

greg0ire commented Oct 18, 2017

The docs currently say:

DATE_ADD(date, days, unit) - Add the number of days to a given date. (Supported units are DAY, MONTH)

please change that too (add your units, and change "number of days")

Member

greg0ire commented Oct 18, 2017

Closes #5835 ?

SebLevDev and others added 2 commits

October 29, 2017 21:48


Add support second, minute, week, year on DATE_ADD and DATE_SUB

f277eef


Improve tests for DATE_ADD and DATE_SUB

So that we can do proper assertions and cover all the possibilities
of the functions.

lcobucci approved these changes

View reviewed changes

lcobucci added this to the 2.6.0 milestone

lcobucci merged commit a82f6c5 into doctrine:master

Majkl578 mentioned this pull request

Tests failing in February (QueryDqlFunctionTest::testDateAdd with "month" dataset) #7031

Closed

JoppeDC commented Jun 3, 2019

This wasn't in the docs yet, leading me to a search for how to do it. Luckily i found this old PR. I've created a PR to add this to the docs.
#7729

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