Problem/Motivation

Unsilenced deprecation notices (3)

  1x: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated
    1x in NodeAccessTest::testQueryWithBaseTableJoin from Drupal\Tests\node\Kernel

  1x: strpbrk(): Passing null to parameter #1 ($string) of type string is deprecated
    1x in NodeAccessTest::testQueryWithBaseTableJoin from Drupal\Tests\node\Kernel

  1x: strtoupper(): Passing null to parameter #1 ($string) of type string is deprecated
    1x in NodeAccessTest::testQueryWithBaseTableJoin from Drupal\Tests\node\Kernel

This happens when you pass a Condition to the query through methods other then the ::condition method, specifically there ::where method. DatabaseAPI "supports" this in implementation but its currently undefined in the interface which only accepts a string to ::where. PHP 8.1's string deprecation exposes that this support was contingent on str_* functions treating the unset NULL operator value as an empty string rather then explicitly ignoring the operator.

Views can trigger this with this code:

  $sub_group->where($clause['field'], $clause['value']);

#1349080: node_access filters out accessible nodes when node is left joined also ran into this with its patch https://www.drupal.org/project/drupal/issues/1349080#comment-14496766

root cause

Steps to reproduce

As far as I know requires custom code, no built in way of triggering.

Proposed resolution

Update documentation to reflect supporting passing a Condition as the field where method calls.
Fix operator detection in ConditionInterface branch of the operator

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 screenshot.png263.74 KBmxr576

Issue fork drupal-3277602

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

mxr576 created an issue. See original summary.

mxr576 opened merge request !2185

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
StatusFileSize
new screenshot.png263.74 KB
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Lendude added a different fix here, I wonder which fixes this problem... :) Maybe both? The code is pretty complex...

https://www.drupal.org/project/drupal/issues/1349080#comment-14497447

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Title: str* functions: Passing null to parameter is deprecated » Handle better when $condition['field'] is a ConditionInterface
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

As @alexpott pointed out on slack, we need to figure out why \Drupal\KernelTests\Core\Database\SelectComplexTest::testJoinConditionObject does not cause the same problem, since it LOOKS like that uses the same code path as the new test in #1349080: node_access filters out accessible nodes when node is left joined

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

neclimdul’s picture

neclimdul
He/Him
commented

If we look at this documentation in compile we'll see why this happens.

          // If the operator and value were not passed in to the
          // @see ConditionInterface::condition() method (and thus have the
          // default value as defined over there) it is assumed to be a valid
          // condition on its own: ignore the operator and value parts.

Technically, this is a valid assumption because its the only documented way of addng as condition other then directly hacking the conditions property through the conditions method. We only have to handle the default values because condition takes case to ensure operator has a valid value. However, Views breaks this assumption in Drupal\views\Plugin\views\query\Sql::buildCondition() where it adds the condition using Condition::where like this:

  $sub_group->where($clause['field'], $clause['value']);

This looks innocuous even if you know that 'field' is a Condition because conditions are stringable. But because the type isn't coerced it ends up triggering this compile code and since ::where forces the operator to null which isn't handled it ends up triggering these string function deprecation messages.

This should be easy enough to duplicate with a view argument like taxonomy but I haven't narrowed that down yet.

daffie’s picture

daffie commented
Status: Active » Needs work
Issue tags: +Needs issue summary update, +Needs tests, +Bug Smash Initiative

What I am missing is how exactly the Database API is failing. Somehow views is using the Database API in a way that makes it fail. We need the IS to be updated with how the views module is using the Database API and how it is failing. We also need a Kernel test for the Database API that will fail without the fix. If the fix is a Database API addition than we also need a CR.

daffie’s picture

daffie commented
Priority: Normal » Major

This issue is blocking #1349080: node_access filters out accessible nodes when node is left joined. As that issue is a major and this is blocking it, makes this issue also a major issue.

neclimdul’s picture

neclimdul
He/Him
commented
Issue summary: View changes

Second pass at a comment. Reading the patch in the other issue I think I'm understanding the changes in the merge and also what y'all are here for and its a completely different scope then I expected from the summary. I filled in how the deprecation messages are being triggered but I don't think that's what this issue is actually is if its blocking the other issue. Y'all are talking about adding a new feature of allowing ::where to explicitly accept a condition in the snippet which is currently not the case and an undefined behavior that kinda works.

I expect there is some testing and documentation updates needed to support that. My comment about the docblock in #8 is part of that documentation but also, the type and documentation for ConditionInterface::where also should be updated to be explicit about this. And I guess that 'value' is actually an addition to support that that would trigger a CR?

To that end, is it possible the other issue could be unblocked by using ConditionInterface::condition()?

daffie’s picture

daffie commented

@neclimdul: I have looked at the patch from the other issue. I see that there is a change to the Database API, only I do not get why the change is necessary. If there is something wrong with the Database API, than lets fix that first. Lets write a simple test (a kernel test for the Database API) that shows what is wrong. I am missing that. After this issue is fixed, we can fix the bug in the views module.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.