Problem/Motivation

Calling \Drupal::service('locale.storage')->deleteStrings() never seems to work.
It fails with this error:

Drupal\Core\Database\InvalidQueryException Calling Drupal\Core\Database\Query\Condition::condition() without an array compatible operator is not supported. See https://www.drupal.org/node/3350985.

The problem is that dbDelete() loops on provided values and adds a condition to the query for each value but it uses a = condition so it only works with scalar values.
deleteStrings() calls dbDelete() with an array of LIDs as value, so it tries to add a query condition with the = operator and an array as value, which does not work.

(This method does not seem to be used by core itself.)

Steps to reproduce

\Drupal::service('locale.storage')->deleteStrings(['lid' => 1]);

Proposed resolution

StringDatabaseStorage::dbDelete() should use an IN condition.
This way StringDatabaseStorage::deleteStrings() can call it with multiple LIDs and they will all be deleted.

In order to keep compatibility with other methods that call dbDelete(), we can check if the value is scalar and create an array that contains only this scalar value that we use in the IN condition.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3497647

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff’s picture

prudloff commented
Issue summary: View changes
quietone’s picture

quietone commented
Version: 11.0.x-dev » 11.x-dev

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

prudloff opened merge request !11306

prudloff’s picture

prudloff commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Can the issue summary be flushed out some please

Proposed solution is a little vague, don't need code examples but how is this being solved.

Also recommend leaving all summary headers, if they are blank or NA.

prudloff’s picture

prudloff commented
Issue summary: View changes
Status: Needs work » Needs review

Tried to improve issue summary.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Much better!

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work
prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I moved the test to a kernel test.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback of switching to a kernel test appears to be addressed

xjm’s picture

xjm
she/her
Primary language English
commented

I queued the test-only changes job (which someone should have done and documented before this was RTBC).

xjm’s picture

xjm
she/her
Primary language English
commented

The test-only job failed as expected (with the error as reported in the original issue):

https://git.drupalcode.org/issue/drupal-3497647/-/jobs/5279091

Drupal\Core\Database\InvalidQueryException: Calling Drupal\Core\Database\Query\Condition::condition() without an array compatible operator is not supported. See https://www.drupal.org/node/3350985
/builds/issue/drupal-3497647/core/lib/Drupal/Core/Database/Query/Condition.php:115
/builds/issue/drupal-3497647/core/lib/Drupal/Core/Database/Query/QueryConditionTrait.php:27
/builds/issue/drupal-3497647/core/modules/locale/src/StringDatabaseStorage.php:530
/builds/issue/drupal-3497647/core/modules/locale/src/StringDatabaseStorage.php:209
/builds/issue/drupal-3497647/core/modules/locale/tests/src/Kernel/LocaleStringTest.php:277
ERRORS!
xjm’s picture

xjm
she/her
Primary language English
commented

I reviewed the relevant APIs, and I agree that this is a good fix. Saving credits.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Sorry, just caught a couple assertion messages creeping back in. @prudloff, are you okay with the inline comments I proposed instead, in place of your assertion messages? If you accept the suggestions, the issue can be moved straight back to RTBC (no need for an extra review cycle for it).

prudloff’s picture

prudloff commented
Status: Needs work » Reviewed & tested by the community

Sorry I didn't know that assertion messages should not be used. (Maybe a phpcs rule delicould catch that?)
I am OK with your changes.

  • xjm committed f1646309 on 11.x 
    Issue #3497647 by prudloff, xjm, quietone: StringDatabaseStorage::...

  • xjm committed ca266be9 on 11.2.x 
    Issue #3497647 by prudloff, xjm, quietone: StringDatabaseStorage::...

  • xjm committed 4e17b5f8 on 10.5.x 
    Issue #3497647 by prudloff, xjm, quietone: StringDatabaseStorage::...
xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

@prudloff, it's one of those "cultural knowledge" things where it's been enforced as a best practice in the core queue and been the subject of repeated massive codebase cleanups, but the policy draft to formalize it has stalled out for years. :) Tried to get the latest version of it moving again today in #3131946: [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages . Unfortunately there are a subset of cases where messages are needed (the policy draft in the linked issue outlines them), so enforcing it with a rule isn't as simple as some.

In any case, thanks for the bug fix! I went back and forth on the priority, but I think this is actually major, given that the method as it stands is basically straight-up broken.

Committed to 11.x, 11.2.x, and 10.5.x as a patch-safe major bug fix. Thanks!

xjm closed merge request !11306

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 11.x-dev » 10.5.x-dev

 

  • catch committed c50865d3 on 10.6.x authored by xjm 
    Issue #3497647 by prudloff, xjm, quietone: StringDatabaseStorage::...

Status: Fixed » Closed (fixed)

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