Problem/Motivation
In #2997748: Views query alter for fields stored in dedicated tables is not working properly we added the possibility to use 'left_formula'
instead of 'left_field'
when creating a views join in code. However, the constructor of the base plugin still appears to always assume a 'left_join'
option there:
https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/view...
$this->leftField = $configuration['left_field'];
While the documentation of the constructor mentions 'left_field'
is unnecessary when 'left_formula'
is used.
* If an SQL expression is needed for the first part of the left table join
* condition, 'left_formula' can be used instead of 'left_field'.
The result is that if we create a join in code using only 'left_formula'
, we get:
Notice: Undefined index: left_field in Drupal\views\Plugin\views\join\JoinPluginBase->__construct() (line 269 of /var/www/html/web/core/modules/views/src/Plugin/views/join/JoinPluginBase.php)
Steps to reproduce
Create a join in code using one of the examples from the plugin documentation, for example:
* If an SQL expression is needed for the first part of the left table join
* condition, 'left_formula' can be used instead of 'left_field'.
* For this SQL:
* @code
* LEFT JOIN {two} ON MAX(one.field_a) = two.field_b AND one.field_c = 'some_val'
* @endcode
* Use this configuration:
* @code
* $configuration = array(
* 'table' => 'two',
* 'field' => 'field_b',
* 'left_table' => 'one',
* 'left_formula' => 'MAX(one.field_a)',
* 'operator' => '=',
* 'extra' => array(
* 0 => array(
* 'left_field' => 'field_c',
* 'value' => 'some_val',
* ),
* ),
* );
* $join = Views::pluginManager('join')->createInstance('standard', $configuration);
* @endcode
Execute your query, observe the PHP notice being thrown.
Proposed resolution
Do not assume 'left_field'
will always be there in the constructor.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#7 | 3221933-TEST-ONLY.patch | 756 bytes | marcoscano |
Issue fork drupal-3221933
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:
Comments
Comment #3
marcoscanoNot sure if we need tests for this type of trivial fix. I'm guessing not, but 🤷♂️
Comment #4
owenbush CreditAttribution: owenbush at Lullabot commentedThis looks good to me.
I create the following to test this out locally:
This adds an additional LEFT JOIN to the core comments view. Like so:
LEFT JOIN {users_field_data} "users_field_data" ON UPPER(users_field_data.mail) = users_field_data.mail
It is completely benign to do so but it worked as a test base for this issue. Immediately after adding the additional join I saw the following in my watchdog log
Notice: Undefined index: left_field in Drupal\views\Plugin\views\join\JoinPluginBase->__construct() (line 269 of /app/web/core/modules/views/src/Plugin/views/join/JoinPluginBase.php)
I then applied the patch from MR 892 and the watchdog notice no longer happens.
This looks good to me. Moving to RTBC.
Comment #5
alexpottWe need to test this. Also do we need either a left field or a left formula and should we test for this? Maybe the current notice has helped someone configure a join correctly in the past.
Comment #6
alexpottFWIW changing a test to cause this is very simple - we can
unset($configuration['left_field']);
in \Drupal\Tests\views\Kernel\Plugin\JoinTestComment #7
marcoscano@alexpott thanks for the review and the pointer for the test!
Attached the fail patch with the test, and I've updated the MR with the same.
Regarding
I'm not 100% sure this is what you had in mind, but I added a check that will throw an exception if a join is instantiated without either `left_field` or `left_formula`. Now that we are not throwing a notice if `left_field` is empty, it could happen. Let me know if that's not what you had in mind.
Thanks!
Comment #8
marcoscanoComment #9
alexpottFor the above review comment.
Comment #10
marcoscano👍Makes sense. MR updated.
Comment #11
owenbush CreditAttribution: owenbush at Lullabot commentedI still had this all set up as I did in comment #4. The patch still applies cleanly and resolves the PHP notice issue. This seems fine to me to go back to RTBC.
Comment #12
alexpottCommitted and pushed 0926b0cc7c to 9.3.x and 3663cc5b9d to 9.2.x. Thanks!
Comment #15
alexpottAs a risk-free bug fix backported this to 9.2.x