Find an alternative to trigger_error in Drupal\Core\Database\Query\Condition::compile

Maybe this one is as simple as this,:we trigger_error E_USER_WARNING instead of E_USER_ERROR. As far as logging is concerned, errors and warnings are either reported both, or none. So we are not changing visibility of the issue in the db log of non-prod sites, just changing the severity level of the message.

If the objective of the trigger_error is to add a log entry for site admins to review, however, then this is not fully right as the errors/warnings are only added if the appropriate setting allows - which is discouraged in prod sites. Better logging directly to a logger service.

The proposed changes make sense, could the issue summary be updated to include that? And think there's anyone to ping for 2nd eyes?

For issue summary update

I'll ask for 2nd eyes on Slack.

Was there a decision to not pursue #4 further and have this logged "properly" to a logger channel?

The other, alternative approach here would be to avoid trigger_error() completely and log a message through the logger, instead. An example in that sense is in 📌 Find an alternative to trigger_error in Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl Active . We need to take a direction, here.

This is just swapping one deprecation for another?

-				#^Call to deprecated method expectError\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
+				#^Call to deprecated method expectWarning\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:

Can we validate the operator earlier, in a more suitable place to inject a logger and/or throw an exception?

c/p from Slack

my intention would be to backfill expectWarning in the issue where we drop the bridge. It’s there already. We could easily backfill expectError, too. But I thought that where for error we should comply to PHPUnit direction, for warning instead we could keep it to reduce the refactoring needed, that alas in this case covers runtime code too, not just test one.

📌 Log when invalid input is swallowed by database layer Closed: outdated makes the same point ten years ago.

This is an exceptional case. Maybe we just throw an exception, and be done with it?

#12 mmm… there are comments in the code above the change explaining why exception should be avoided here

Is the comment correct though? The error is thrown in compile() and __toString() explicitly requires you to call compile() first:

  public function __toString() {
    // If the caller forgot to call compile() first, refuse to run.
    if ($this->changed) {
      return '';
    }
    return $this->stringVersion;
  }

It also looks like this check could be moved into condition(); $conditions is protected and this is the only place it is added to with user input.

#14 sounds sensible.

samit.310@gmail.com → made their first commit to this issue’s fork.

Implemented #14 in MR!7396.

This was the fix for #2492967: SQL layer: $match_operator is vulnerable to injection attack → and the test coverage should be good enough, but reviewers should have a quick look to make sure we haven't missed anything here. It's not clear to me why this was added in compile() instead of condition().

mondrake → closed merge request !6489

A kernel test is failing.

Turns out we have some near-duplicate test coverage, which I think we should keep, but we can clean it up somewhat here.

longwave → changed the visibility of the branch 3419690-find-an-alternative-to-trigger-error to hidden.

Based on issue summary update MR appears to match proposed solution and am seeing all green.

Believe this is good.

I've added a comment to the MR - I don't think we should be moving the location of the check.

The compile method now includes the detection of potentially dangerous operators. If any such operators are found during compilation, it will throw an InvalidQueryException with a message indicating the issue. This way, the error handling is centralized in the compile method, addressing your concerns about moving the condition out of compile. Please review

Reworked as per #27.

Replacement of trigger_error for InvalidQueryException() seems good.

Nothing has broken.

Read through the MR first and had various questions about the claims in the comment we're removing, then read the discussion on here and it answered all those questions.

Committed/pushed to 11.x, thanks!

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