Problem
In a config entity query, when you add an IS NOT NULL
condition (e.g., with exists()
) on a field, but the entity's field value has an array value for that field, a warning will be triggered:
Warning: mb_strtolower() expects parameter 1 to be string, array given in Drupal\Component\Utility\Unicode::strtolower() (line 326 of core/lib/Drupal/Component/Utility/Unicode.php).
This happens because of this code in \Drupal\Core\Config\Entity\Query\Condition::match()
:
// We always want a case-insensitive match.
if (!is_bool($value)) {
$value = Unicode::strtolower($value);
}
Obviously, an array is no boolean value, but we also don't want to convert it to lowercase. Convert its values to lowercase, maybe – that would also support matching array field values to array condition values. Not sure if that should be supported, but I don't see any reason not to at least fix it for IS NOT NULL
.
Example scenario
I have a config entity with several plugin settings in a property, with this structure:
PROPERTY => [
PLUGIN_ID => [
# PLUGIN_SETTINGS
],
PLUGIN_ID => [
# PLUGIN_SETTINGS
],
# …
]
Now, if I want to find entities that use a certain plugin, I need exactly this: an entity query with ->exists('PROPERTY.PLUGIN_ID')
, where the value found for the field will be an array.
Comment | File | Size | Author |
---|---|---|---|
#15 | 2899014-15.patch | 2.97 KB | Dinesh18 |
#15 | interdiff.txt | 864 bytes | Dinesh18 |
#12 | 2899014-12.patch | 2.98 KB | Dinesh18 |
#12 | interdiff.txt | 961 bytes | Dinesh18 |
#6 | 2899014-6--config_entity_query_isnotnull_condition_array_value.patch | 2.99 KB | drunken monkey |
Comments
Comment #2
drunken monkeyThis should do it.
Comment #3
tstoecklerFix looks good to me, thanks!
I think we can do the same for
IS NULL
, though, as well. Also would be nice to add a quick test. I see we already have\Drupal\KernelTests\Core\Entity\ConfigEntityQueryTest::testConfigEntityQuery()
which does some testing with->exists()
so hopefully it's not too much work.Comment #4
tstoecklerComment #6
drunken monkeyYou're right, that's really an easy addition. See attached.
Comment #7
tstoecklerComment #9
tstoecklerThanks the patch looks great, including the added test coverage. I have one minor quibble:
I think the putting
$is_set
into a variable is a bit unnecessary.Would be awesome if you could quickly fix that, then I can RTBC.
Comment #10
tstoecklerAlso, if you don't mind, it would be nice to add the
third parameter here, to ensure this actually does what it should. Otherwise this will return unexpected things when e.g. integer 1 is passed as
$condition['operator']
.Comment #11
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedComment #12
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is an updated patch and interdiff which implements #9 and #10
Comment #13
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedComment #14
tstoecklerThanks @Dinesh18!
The patch looks great, but I guess I failed at explaining what I meant in #10.
I put the
part in my previous comment only to explain what the third parameter is for, it's not actually needed in the code. So where you put
, you can just put
TRUE
, that would be perfect.Thanks again and sorry for not taking the time to explain this properly earlier.
Comment #15
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is the updated patch and interdiff which implements #14
Comment #16
tstoecklerThat's perfect, thank you!
Comment #20
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #21
xjmI would have just written this as
Way less complicated logic and easier to read, and no creating both an array and a scalar we don't need.
Comment #22
tstoeckler#21 is missing two isset()s. Either way would be fine by me, but I actually find the way it went in clearer.