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

Issue fork drupal-3221933

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcoscano created an issue. See original summary.

marcoscano’s picture

Assigned: marcoscano » Unassigned
Status: Active » Needs review

Not sure if we need tests for this type of trivial fix. I'm guessing not, but 🤷‍♂️

owenbush’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

I create the following to test this out locally:

/**
 * Implements hook_views_query_alter().
 */
function MY_MODULE_views_query_alter(ViewExecutable $view, QueryPluginBase $query) {
  if ($view->id() === 'comment' && $view->current_display === 'page_published') {
    $configuration = [
      'table' => 'users_field_data',
      'field' => 'mail',
      'left_table' => 'comment_field_data',
      'left_formula' => 'UPPER(users_field_data.mail)',
      'operator' => '=',
    ];
    $join = Views::pluginManager('join')->createInstance('standard', $configuration);
    assert($query instanceof Sql);
    $query->addRelationship('users_field_data', $join, 'comment_field_data');
  }
}

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We 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.

alexpott’s picture

FWIW changing a test to cause this is very simple - we can unset($configuration['left_field']); in \Drupal\Tests\views\Kernel\Plugin\JoinTest

marcoscano’s picture

@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

Also do we need either a left field or a left formula and should we test for this?

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!

marcoscano’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
alexpott’s picture

Status: Needs review » Needs work

For the above review comment.

marcoscano’s picture

Status: Needs work » Needs review

👍Makes sense. MR updated.

owenbush’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 0926b0cc7c to 9.3.x and 3663cc5b9d to 9.2.x. Thanks!

  • alexpott committed 0926b0c on 9.3.x
    Issue #3221933 by marcoscano, alexpott, owenbush: PHP Notice when using...

  • alexpott committed 3663cc5 on 9.2.x
    Issue #3221933 by marcoscano, alexpott, owenbush: PHP Notice when using...
alexpott’s picture

Version: 9.3.x-dev » 9.2.x-dev

As a risk-free bug fix backported this to 9.2.x

Status: Fixed » Closed (fixed)

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