FIX Don't throw exception for bad dates with mysql 8.0 #312
Conversation
| @@ -66,6 +67,7 @@ public function testInvalidDateFormat() | |||
| */ | |||
| private function isRunningMySQL() | |||
| { | |||
| return strpos(strtolower(get_class(DB::get_connector())), 'mysql') !== false; | |||
| $connector = DB::get_connector(); | |||
| return strpos(strtolower(get_class($connector)), 'mysql') !== false || $connector instanceof PDOConnector; | |||
There was a problem hiding this comment.
I think postgres can also run PDO? (we don't test this in our matrix though)
There was a problem hiding this comment.
Hmmm that's a good point... since we don't have postgres PDO in our matrix I think I'd prefer to keep this change - the likelyhood of someone running this test with postgres PDO is very unlikely (and they can find a better way to do this if they want to) - but for now we do have mysql PDO tests.
There was a problem hiding this comment.
so how come strpos(strtolower(get_class(DB::get_connector())), 'mysql') isn't working? What does DB::get_connector() return with mysql pdo?
There was a problem hiding this comment.
The PDO connector class is literally PDOConnector (the full namespaced class name is checked of course, so SilverStripe\ORM\Connect\PDOConnector)
So it's a mysql db, but the connector class name doesn't hint at that at all.
| * The request has had an error that can result in a DB exception | ||
| * if filtering by the dates provided. | ||
| */ | ||
| public function requestHasBadDates(): bool |
There was a problem hiding this comment.
New API, would need to target 2 so doesn't work in terms of fixing the unit test on the previous branch
There was a problem hiding this comment.
In general for normal circumstances I'd agree. But I don't think this is going to cause any problems for anyone, and it will fix this bug.
I think we either need to accept that this is not how we normally treat semver, but since it won't break anything it's fine, or target to 2 and merge there, and accept that anything earlier will not pass in CI.
There was a problem hiding this comment.
Unless you can think of a way to do this without adding the new method?
Would marking it internal help resolve any trepidation?
It's not just the test though. The test is correctly failing, because the functionality is broken. This PR aims to fix the broken functionality. |
GuySartorelli
force-pushed
the
pulls/2.9/fix-mysql8-error
branch
from
0e4ac66
to
ba29f2f
Compare
|
Closing in favor of #311 |
Follow-up from #310
Fixes https://github.com/silverstripe/recipe-solr-search/runs/7352246945?check_suite_focus=true
Bit of an odd fix as it does add API, but I think that's unavoidable in this situation.
Parent issue