Sql Server is unable to directly compare a blob to a string without casting the string to varbinary. Core database tests do not test that blobs can be used in a condition, so this does not appear to be an expectation of databases. This test can be easily modified while maintaining test integrity by changing:
$logged = (bool) Database::getConnection()->select('watchdog')
->fields('watchdog', ['wid'])
->condition('type', 'system')
->condition('message', "View display '@id': Comment field formatter '@name' was disabled because it is using the comment view display '@display' (@mode) that was just disabled.")
->condition('variables', serialize($arguments))
->execute()
->fetchField();
$this->assertTrue($logged);
To:
$logged = Database::getConnection()->select('watchdog')
->fields('watchdog', ['variables'])
->condition('type', 'system')
->condition('message', "View display '@id': Comment field formatter '@name' was disabled because it is using the comment view display '@display' (@mode) that was just disabled.")
->execute()
->fetchField();
$this->assertEquals(serialize($arguments), $logged);
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3134475-6.patch | 1.19 KB | hardik_patel_12 |
Comments
Comment #2
beakerboyA patch for review.
Comment #4
beakerboyCore is failing on its own for the same reason. I’ll manually retest when core passes.
Comment #5
alexpottComment #6
hardik_patel_12 commentedRe-roll for 8.9.x-dev.
Comment #7
andypostIirc it depends on schema definition, I mean deserialization from blob
Comment #8
beakerboy@andypost I don’t understand what you are suggesting. PDO is set to convert everything that is pulled out of the database to a string, so Varbinary data can be extracted and compared to the serialized array just fine with sql Server, and every Database. The problem is with the server performing the comparison between the binary data in the database and the string bound to the placeholder. This is why the condition was removed from the query and instead converted into the assertEquals() comparison.
Comment #9
alexpott@andypost yeah it seems that the watchdog table was create before the option to do
'serialize' => TRUE,in the schema existed. Ideally it would use it. But that's not the case here. In think this change is fine.Comment #10
alexpott@Hardik_Patel_12 before rerolling a patch please check that it is necessary #6 is identical to #2.
Comment #11
daffie commentedWhat needs to be tested in the CommentIntegrationTest is with this patch still tested.
The way that is was just happens to work for the by core supported databases.
All the code changes look good to me.
For me it is RTBC.
I leave it up to the committer which patch to commit. I see no difference between the two patches.
Comment #12
hardik_patel_12 commented@alexpott thank you for clarifying my doubts on slack regarding we can trigger tests on single patch across multiple branches , so no need to create new patch if there is no code changes. I'll keep it in mind.
Comment #13
alexpottCommitted and pushed 9d8849513f to 9.1.x and 213fbc8e18 to 9.0.x and 3d3aab2e7f to 8.9.x and dc56cb8e1e to 8.8.x. Thanks!
Backported to 8.8.x as this is a test-only bug fix.