[PHP 8.1] Deprecated function: strpos(): Passing null to parameter #1 LayoutBuilderUiCacheContext.php on line 28

@afsch could you add more steps how to reproduce. Thanks.

Plz add Steps to reproduce @afsch

Updating patch go through this once and found similar https://www.drupal.org/project/drupal/issues/3312931

Thanks. It worked for me.

Re-Roll #6

For me it happens when I run certain Drush commands. I guess the route matcher cannot determine the route (?)

Thanks folks for working on this.

Removing credit for an apparently unnecessary MR and rerolls, and hiding incorrect MRs and patches. (Right now, we have four different approaches in about as many patches and MRs.)

Rather than trying to win the cleverest line of code award with multiple string-manipulation function calls and typecasts, how about we actually just untangle the logic and write out a proper if/else?

Both of these lines trace to #3002608: Remove contextual links not related to layout administration inside layout builder blocks and you can see discussion of them on that issue as well.

Thanks!

Did not mean to change the branch, though.

Adding credit for @claudiu.cristea for providing potential STR.

@Rajeshreeputra, you just appear to have merged commits without addressing my feedback in #17.

This patch broke a few contextual links for me. That is because the strpos() === 0 means that the needle is expected in the beginning of the string. Changing this to stropos() === FALSE almost negates entirely the condition.

Please review comment #17, in which I asked us to write this out as an if-else.

No one should push further commits unless they are attempting to implement that feedback. Thanks!

@xjm I am really sorry I missed your comment, I looked into it now. I was just trying to fix the logic error.
I have committed an attempt to untangle it but I am not sure which part do you mean. I tried to make it clearer. If I was wrong, please, let me know what do you need.

The base branch is 10.0.x

No idea what's the issue with the Composer failure

The Composer error is due to some infrastructural issues currently with testing. See here:
https://twitter.com/drupal_infra/status/1613597701215776768

Temporary patch of the latest diff to get tests to run.

Thanks @idimopoulos, that's on the right track! I'll continue to post comments on the MR, as hopefully we can go back to using it once the testing issues are resolved.

Posted some suggestions to further clarify the lines so we don't get this confusion in the future.

This is a bug and needs to be addressed in 9.5.x

Implemented the fix and addressed @xjm remarks in merge request 3587

From what I can tell MR 3573 is the same as MR 3572 which might be the same as 3143 before it got borked with D10 changes.

MR 3587 seems to be the correct approach but feels it will need test case.

Test what? This is just a tiny refactoring because of PHP 8.1 notices

Re #39, #40: There's already test coverage in \Drupal\Tests\layout_builder\Functional\LayoutSectionTest::testLayoutSectionFormatter()

Afaics some of the fixes being applied here were done in #3324574: Deprecated function: strpos() in layout_builder_entity_view_alter.

There are way too many MRs on this issue but also AFAICS the fix to LayoutBuilderUiCacheContext won't actually fix the problem. I think the fix should be something like;
return 'is_layout_builder_ui.' . (int) !str_starts_with($this->routeMatch->getRouteName() ?? '', 'layout_builder.');

i.e. adding ?? '' to cope with situation where getRouteName() returns a NULL. It'd be interesting to understand when this function is being called at there is no route match.

@alexpott, The idea of casting to (int) was rejected by @xjm in #17, #21, #24. She insisted to switch to an if (...) block

Hiding patches

Discussed in Slack with @alexpott and will rebase against 10.1.x

Re #42

I think the fix should be something like;
return 'is_layout_builder_ui.' . (int) !str_starts_with($this->routeMatch->getRouteName() ?? '', 'layout_builder.');

i.e. adding ?? '' to cope with situation where getRouteName() returns a NULL. It'd be interesting to understand when this function is being called at there is no route match.

Discussed with @alexpott on Slack and we agreed that the fix, currently in merge request 3587, is correct and fulfills also the requirement from #17.

This is ready for a final review.

Following the discussion in slack, it seems like this doesn't need a test so the fix looks good to me.

Committed 341af8bf30 and pushed to 10.1.x. Thanks!
Committed and pushed 9dd37aa3ee to 10.0.x and 067b544dfd to 9.5.x. Thanks!

Discussed whether this needs tests with @catch and @alexpott
Catch said if it were a task to use str_starts_with we wouldn't need it, so the only question is the extra if.
@alexpott wondered if this is resolved by #3324574: Deprecated function: strpos() in layout_builder_entity_view_alter

The consensus was probably doesn't need a test

crosspost, sorry

Somehow this got unpublished

Hey All,

I've got a instance where this merge request is failing upon Composer update and I'm wondering is anyone else is running into that issue. Below is the error I'm getting from composer, running on Acquia.

    https://git.drupalcode.org/project/drupal/-/merge_requests/2601.patch (3301770 - Passing null to parameter to strpos() is deprecated with PHP8.1)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/2601.patch

In PluginManager.php(274) : eval()'d code line 331:
                                                                                                              
  Cannot apply patch 3301770 - Passing null to parameter to strpos() is deprecated with PHP8.1 (https://git.  
  drupalcode.org/project/drupal/-/merge_requests/2601.patch)!     

I'm just running composer install or composer update and getting this issue.

Thanks all.

@jmester13, there are new releases for 9.5, 10.0 and 10.1. You don't need the patch anymore, just use the latest Drupal version.

@claudiu.cristea

Thanks for the quick reply. I am already on 9.5.3.

I've ensured I don't have this patch in my composer file and have removed the composer.lock file and vendor directory.

I've actually tried removing the few core patches I had and am still getting this error.

Then I removed all my patches, vendor, composer.lock and removed the composer caches.

Still getting the error and seeing that the composer.lock that is generated contains the core patch.

Seems I have an oddity, any chance anyone else has been in this position. If not I'll try and check in with the folks at Acquia and see if it's something that is system related. But it almost seems that core is failing to patch itself or something?

Latest Composer log below.

Thanks all.

- Installing phpunit/phpunit (9.6.5): Extracting archive
- Applying patches for drupal/core
https://www.drupal.org/files/issues/2019-12-28/3082690-80.patch (3059955 - It is possible to overflow the number of items allowed in Media Library)
https://www.drupal.org/files/issues/2022-11-11/3222107-2.patch (3222107 - jQuery UI library order is incorrect when a large number of javascript files is loaded between two jQuery UI libraries)
https://git.drupalcode.org/project/drupal/-/merge_requests/2598.patch (3301692: Passing null to parameter to mb_strtolower() is deprecated with PHP 8.1)
https://git.drupalcode.org/project/drupal/-/merge_requests/3143.patch (3313342 - [PHP 8.1] Deprecated function: strpos(): Passing null to parameter #1 LayoutBuilderUiCacheContext.php on line 28)
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/3143.patch

In Patches.php line 331:

Cannot apply patch 3313342 - [PHP 8.1] Deprecated function: strpos(): Passing null to parameter #1
LayoutBuilderUiCacheContext.php on line 28 (https://git.drupalcode.org/project/drupal/-/merge_req
uests/3143.patch)!

@jmester13, latest is 9.5.5

Removing credit for MR.

Crediting @acbramley as he contributed with the review