Fix [warning] Invalid argument supplied for foreach() EntityViewsDataTaxonomyFilterTrait.php:26 #575
Conversation
…TaxonomyFilterTrait.php:26 farmOS#575
mstenta
force-pushed
the
2.x-fix-entityviewsdatataxonomyfiltertrait
branch
from
02ce5d5
to
a85b677
Compare
| $field_name = $field->getName(); | ||
| foreach ($data as $table_name => $table_data) { | ||
| foreach ($table_data as $table_field_name => $field_data) { | ||
| if (isset($field_data['filter']) && strpos($table_field_name, $field_name) === 0) { |
There was a problem hiding this comment.
strpos($table_field_name, $field_name) === 0
I'm curious why we can't check that $table_field_name == $field_name ? I think it could be possible that two different fields have the same prefix and this logic could inadvertently change the filter ID for the wrong field.
Consider two field names: Taxonomy reference custom_field and String custom_field_raw. I think this logic would change custom_field_raw to use the taxonomy_index_tid filter.
There was a problem hiding this comment.
but I think this approach should work! 👍
There was a problem hiding this comment.
Consider two field names: Taxonomy reference
custom_fieldand Stringcustom_field_raw. I think this logic would changecustom_field_rawto use thetaxonomy_index_tidfilter.
That's a good point.
I'm not sure why we used strpos($table_field_name, $field_name) === 0 originally. Here is the commit that added it, if it rings any bells: 7358e4d
This PR doesn't change that bit, just restructures the foreach so that we don't need to figure out $table_name.
I wonder if there was a good reason for using strpos... 🤔
There was a problem hiding this comment.
This PR doesn't change that bit, just restructures the
foreachso that we don't need to figure out$table_name.
Ah but I see how that would introduce this issue, as we are no longer limiting which tables this logic can potentially run on...
There was a problem hiding this comment.
Ah ok... just ran this through the debugger and quickly realized why we used strpos()...
Example:
$table_field_nameisanimal_type_target_id$field_nameisanimal_type
There was a problem hiding this comment.
But not for all of them...
Example:
$table_field_nameisunits$field_nameisunits
There was a problem hiding this comment.
Perhaps we could do:
if (isset($field_data['filter']) && in_array($table_field_name, [$field_name, $field_name . '_target_id'])) {
... basically checking if it's either $field_name or $field_name . '_target_id'.
There was a problem hiding this comment.
I think it could be possible that two different fields have the same prefix and this logic could inadvertently change the filter ID for the wrong field.
Ah ha! Sure enough I found a case where this happens in the custom codebase I'm working in, which has both field_input and field_input_date fields.
…TaxonomyFilterTrait.php:26 farmOS#575
mstenta
force-pushed
the
2.x-fix-entityviewsdatataxonomyfiltertrait
branch
from
a85b677
to
bef0400
Compare
|
OK. Force-pushed the commit to implement this change. I also restructured the code to make the logic more readable with extra comments etc. |
Fixes #574