Fix build issues

[greg0ire]

Decorated text used to be wrapped too early in SymfonyStyle->block()
See symfony/symfony#40348

[greg0ire]

Symfony v3 is in security-fixes mode only, that's why the bugfix wasn't contributed to v3

[greg0ire]

The remaining failures looks like it might happen every month, or every year, but will not happen tomorrow.

[greg0ire]

See @lcobucci fighting it 3 years ago in 46c0861 , in a commit hilariously named "Fix date calculation in tests (again)" 🤣

[greg0ire]

>>> (new DateTimeImmutable())->modify('-1 month')
=> DateTimeImmutable {#213
     +"date": "2021-03-02 09:21:48.312300",
     +"timezone_type": 3,
     +"timezone": "Europe/Paris",
   }

but-why.gif

[matks]

The remaining failures looks like it might happen every month, or every year, but will not happen tomorrow.

Hello, it might be naive and maybe I am the 47th person talking about it, but have you considered wrapping time behind an interface so you control it, and use PHP DateTime as an implementation of this interface?

https://www.matheus.ro/2017/09/25/unit-test-time-dependency/

[beberlei]

https://twitter.com/stauffermatt/status/1376699768328630272?s=21

[greg0ire]

@matks that would work for the PHP part, the trouble is, the other thing we are getting a date from is the RDBMS under test. Consider sending a PR in case you know how to fix that.

[matks]

@matks that would work for the PHP part, the trouble is, the other thing we are getting a date from is the RDBMS under test. Consider sending a PR in case you know how to fix that.

😅 I'm afraid that the RDBMS is indeed a much bigger issue to handle.

I will try to look at it but I doubt my experience will be enough.

[greg0ire]

I guess all we have to do is see how subMonthsNoOverflow() in @beberlei 's referenced tweet is implemented.

UPD: easier said than done because of all the magic but in the end I think it boils down to being aware of how many days each months has, which thankfully, does not need to be hardcoded: (new DateTime())->format('t') (see https://www.php.net/manual/en/datetime.format.php)

[greg0ire]

I pushed what I think is a solution but maybe it's not a great solution, since it shows the behavior is not portable, and enforces that non-portability. Maybe a better solution would be to do a rougher assertion… after all, what do we even want to test here? The exact result of DATE_SUB or that this DQL function is mapped to an SQL function that returns something roughly one month ago? If you agree, then maybe we should change this line:

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

Line 354 in 3f3de70

'month' => ['month', 1, $secondsInDay],

to 'month' => ['month', 1, 3 * $secondsInDay] ?

[matks]

I pushed what I think is a solution but maybe it's not a great solution

It seems to be a pragmatic solution.

Moreover, if tomorrow SQLite behavior changes or MariaDB behavior changes, you might need to enforce even more not-portability. You are constrained by the RDBMS, so I dont think you need to give much thought to this topic. The test path is messy, but it's not because it's a bad test. It's because it represents a messy real-life situation 😅 .

[lcobucci]

@greg0ire alternatively you can use a fixed date/time instead of relying on the current db date. That should make things more stable.

[greg0ire]

@lcobucci I love that solution, why didn't I think of that? That way, if we really feel we want to enforce a specific behavior for 30-02-YYYY, we can do so in a separate test, and we will no longer have a test that depends on the current time.

[greg0ire]

Well now Postgresql is unhappy with this :(

14) Doctrine\Tests\ORM\Functional\QueryDqlFunctionTest::testDateSub with data set "second" ('second', 10, 10)
Exception: [Doctrine\DBAL\Exception\DriverException] An exception occurred while executing 'SELECT '2006-04-13 00:00:00' AS sclr_0, ('2006-04-13 00:00:00' - (10 || ' SECOND')::interval) AS sclr_1 FROM company_managers c0_ INNER JOIN company_employees c1_ ON c0_.id = c1_.id INNER JOIN company_persons c2_ ON c0_.id = c2_.id LIMIT 1':

SQLSTATE[22007]: Invalid datetime format: 7 ERROR:  invalid input syntax for type interval: "2006-04-13 00:00:00"
LINE 1: SELECT '2006-04-13 00:00:00' AS sclr_0, ('2006-04-13 00:00:0...

UPD: because CURRENT_TIMESTAMP is a timestamptz. and this is a string.

But then why was it happy with CURRENT_TIMESTAMP()?

[greg0ire]

Reverting to the original solution, but do advise if you know how to use a fixed date in a way that will work with all platforms (so as a string with most, but with ::timestamp appended to it for PostgreSQL).

[ostrolucky]

Not a great solution. Abstraction layer is supposed to attempt to abstract away these database differences. This test demonstrates it's not doing its job. Instead of hiding this fact in test case, solution could be to make sure all databases return same result given same parameters. This can be done perhaps on DateSubFunction level or in DBAL.

But I realize I ask for too much here, so I have different proposition how to avoid this problem every march. Stop testing CURRENT_TIMESTAMP() function, that's not the goal of this testscase and is already covered in multitude of the other tests. In this test case, always use value for first day of current month as a basis for DATE_SUB. Or if you don't like that much, random day in range of 1-28 of current month. Not the current date.

[greg0ire]

@ostrolucky that's kinda what @lcobucci proposed, and I tried implementing it, but failed because of #8573 (comment) . Do you know of a workaround for that?

[lcobucci]

@greg0ire I'd say that it reveals an issue on DBAL, the method that returns the date_* function should cast the string into a timestamp when that's necessary: https://www.db-fiddle.com/f/4jyoMCicNSZpjMt4jFYoz5/1471

[ostrolucky]

Yeah. Workaround might be to have entry with hardcoded date in CompanyManager, then you can use something like DATE_SUB(m.date, .... I can't decide if it's much better though.

[greg0ire]

Ok well, let's go with the ugly solution that works then.