Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
FileSize
11.55 KB
andypost’s picture

It looks good to go, except the one hunk - maybe better to convert message to "is removed in drupal:10.0.0" because otherwise it will be hard to catch

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -606,9 +606,6 @@ protected function filterComment($comment = '') {
   public function query($query, array $args = [], $options = []) {
...
-    if (isset($options['target'])) {
-      @trigger_error('Passing a \'target\' key to \\Drupal\\Core\\Database\\Connection::query $options argument is deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.0. Instead, use \\Drupal\\Core\\Database\\Database::getConnection($target)->query(). See https://www.drupal.org/node/2993033.', E_USER_DEPRECATED);

I think it makes sense to keep this to prevent incomplete conversions of deprecated code

Berdir’s picture

Does it even do anything with it afterwards?

I don't think we need to keep it or change it to D10. What we could maybe do is throw an exception and make it fail hard?

andypost’s picture

Maybe convert it to assert()?

Berdir’s picture

Yes, that sounds like a good idea.

andypost’s picture

andypost’s picture

02:32:19 /var/lib/jenkins/jobs/drupal_patches/builds/20127/log:
02:32:19 Fatal error: Uncaught AssertionError: Passing "target" option to query() has no effect. See https://www.drupal.org/node/2993033 in /var/www/html/core/lib/Drupal/Core/Database/Connection.php:609
02:32:19 [TextFinder plugin] Finished looking for pattern 'Fatal error' in the console output

As setup fails this option is used some places...

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

The postgresql tests fails look unrelated to me, and this looks good overall.

larowlan’s picture

According to https://www.drupal.org/pift-ci-job/1550844 there are no fails in Postgres ATM

Queueing up new test runs to be safe™

larowlan’s picture

Issue summary: View changes
larowlan’s picture

Issue summary: View changes

Actually, those fails are on PHP7.2, which isn't supported on D9 anymore. Queued up a MYSQL run on 7.3

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0281f2f and pushed to 9.0.x. Thanks!

  • alexpott committed 0281f2f on 9.0.x
    Issue #3097879 by andypost, longwave, Berdir, larowlan: Remove all @...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.