Passing null to stripos() and rtrim() is deprecated

I think we can simplify this by explicitly casting the argument since null will return an empty string.

MR created from #3

Also fix somewhere else PHP 8.1 warning related, e.g. rtrim().

@hswong3i

Good catch, so far everything seems fixed and working. We can probably also just check the $path variable for the two optimizer files to reduce code.

With https://git.drupalcode.org/project/advagg/-/merge_requests/6 we could have patch with https://git.drupalcode.org/project/advagg/-/merge_requests/6.diff.

This could simplify the create patch action + interdiff effort ;-)

Confirmed #8 resolved the PHP warnings with fresh Drupal 9.3.x install.

Although the patch makes the error go away (I've tested #8, but it looks very similar to merge request #6), it feels quite bloated to me. Yes, you can no longer pass NULL values to stripos. But this does not mean that every occurence of stripos needs to be cast to string. Only those occurences where the input could contain a NULL value are the 'dangerous' ones.

As far as I can see, line 57 from the ResponseSubsriber.php is the only one throwing warnings. This can be simply rewritten to:

      // Ensure that it is an html response.
      if ($response->headers->get('Content-Type') !== 'text/html') {
        return;
      }

I believe there is no need to check if text/html exists anywhere in the string. There is also no other content type that has the same start, so no need for stripos.

Whoops, I was about to RTBC initially, but after code inspection I think needs review is the better status for now.

Hi Robin,

MDN define Content-Type syntaxe as:

Content-Type: text/html; charset=utf-8

- Do you think this will always work ?

      if ($response->headers->get('Content-Type') !== 'text/html') {

- I totally agree with you +1

this does not mean that every occurence of stripos needs to be cast to string. Only those occurences where the input could contain a NULL value

- We need to decide which ligne of code is "dangerous". I added a patch for another case

-    $dir = rtrim((string) $this->config->get('unified_multisite_dir'), '/');
+    // Skip if null.
+    if (!$this->config->get('unified_multisite_dir')) {
+      return;
+    }

- Isn't type-casting considered to be bad? I added a simple if condition with a simple return;

+      // Skip if null.
+      if ($response->headers->get('Content-Type')) {
+        return;
+      }

The patch from #8 works as expected for me, but it does indeed seem a bit bloated...
I tried the patch from #14 and the warnings remain, so that one is not working for me.

@weseze and @boulaffasae. The patch from #14 contains a mistake. It now returns when a content type is defined. The if-statement should be inverted and should look like this instead:

      // Skip if null.
      if (!$response->headers->get('Content-Type')) { // Note the !
        return;
      }

- Do you think this will always work ?

if ($response->headers->get('Content-Type') !== 'text/html') {

You're right, I looked through Symfony's codebase, but they appear to be using both. Better to be on the save side.
Instead of a separate if-statement, both can also be combined in one if instead.

And for unified_multisite_dir; I was assuming that would have a default value assigned in advagg_mod.settings.yml. But apparently it was never added there, so we cannot rely on a proper default value. It is defined in the schema.yml file as a string though. I have no idea if Drupal will automagically do type conversion there. According to the documentation (bullet 4 on https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...) values should be typecasted to the right type already so that config value should always return a string?

It turns out that the reason I was not seeing any errors coming from advagg_mod was because I did not have that one enabled... The schema definition alone does not prevent the error from occuring. Adding it to the settings.yml file will help for new installations, but not for running installations.

So we can either cast the value to a string in code or run an update hook to update the configuration and write an empty string if no value is defined. I think the latter is the better option. Patch provided.

And now with UTF-8 encoding...

I think this should do the trick!
Patch can be downloaded from: https://git.drupalcode.org/project/advagg/-/merge_requests/6.diff

I can confirm the issue, and that the patch "https://git.drupalcode.org/project/advagg/-/merge_requests/6.diff" solves it for us.

+1 RTBC

+1 RTBC

Then let's do it :)

Which patch will actually resolve this issue? patch 18 or 6.diff, as i tried both and logs still complain.

@Rar9 can you share the error? Is it coming from a sub module of advagg or perhaps some other module?
3 people and me confirm that 6.diff solves the issue.

I had not yet seen the feedback from neclimdul but that seems like a neater solution. I will try to look into that next week.

There is also one more issue in advagg_mod, here:

Deprecated function: rtrim(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\advagg_mod\EventSubscriber\InitSubscriber->onEvent() (line 130 of /var/www/html/web/modules/contrib/advagg/advagg_mod/src/EventSubscriber/InitSubscriber.php)

@janjanjan, the provided patch includes a database update for adding this missing configuration parameter. Did you run that update?

Copy of patch from https://git.drupalcode.org/project/advagg/-/merge_requests/6.diff (#25) attached, to be used with composer, see "Patches from drupal.org merge request URLs are dangerous?".

Thank you for the patch.

Patch works. Thank you.

Would be nice to see it in a release.

This can quickly fill up logs on the server. Using patch for now. Let's hope this gets released ASAP

Thank you for the patch.

Patch from #28 does not solve this for me with current drupal 9.4.2 and advagg 4.1. Still getting a lot of rtrim errors. I included the patch via composer-patches.

Hi,

Patch from #28 does solve this BUT with current drupal 9.4.1 and advagg 4.1 I had to run DBupdate after patching with composer.
(that was new to me, that after patching a module I have to run dbupdate so I guess someone else might also miss that)

Thanks +1 for the patch

Please try these patches for 5.0.x

Hi @thalles

I have just tested this and the issue still exists in 5.0.0. The current patch applies cleanly. The only change that is needed is that the update number should be changed. But I am not sure what the number format should be with the new versioning format. Something like 5001 maybe?

I've looked for some existing documentation and there is a mentioning here https://www.drupal.org/docs/distributions/degov/semantic-versioning-mode... however the core issue #3010334: Document how contrib hook_update_N() should be numbered now that modules can be compatible with multiple major branches and versioned semantically is still undecided.
5001 won't work because it is lower than the current update numers. So for now I suppose we better stick to the old standard [Major Core][Major Module][Serial] structure. The update number would become 9501 then because 5.x is for Drupal 9.

PR updated.

I think this is still RTBC. Just take above comment into consideration. Update number 50001 should also work.

Indeed it is RTBC, we are using the patch on the production website for a while now.

I think this should get into a release asap as users not using the patch will get their PHP logs quickly filled.

For the most part the patch from #28 worked for me. But some stubborn sites still didn't import the correct config settings.

So I modified the patch so it doesn't necessarily rely on the database update.

I think some maintainer should merge this into 5.x

+1
Merge into 5.x would be great.

These things could be fixed on commit.

1. +++ b/advagg_mod/advagg_mod.install
   @@ -28,6 +28,20 @@ function advagg_mod_update_8301() {
   +  if ($config->get('unified_multisite_dir')) {
   

   I had a null value in my site's config for this property. This check should check for is_string or scalar, not just if there is a value. Because if this update where ever re-run again, an empty string returns false and isn't re-entrant.

2. +++ b/advagg_mod/src/EventSubscriber/InitSubscriber.php
   @@ -127,7 +127,8 @@ class InitSubscriber implements EventSubscriberInterface {
   +    $multisiteDir = $this->config->get('unified_multisite_dir') ?? '';
   

   This isn't needed with the update hook.

Patch in #40 worked as expected

Patch #40 worked as expected.

Drupal 9.4.5
Advagg 8.x-4.1

+++ b/advagg_mod/advagg_mod.install
@@ -28,6 +28,20 @@ function advagg_mod_update_8301() {
+  if ($config->get('unified_multisite_dir')) {
+    return;
+  }
+  else {

Replace these 4 lines with:
if (!is_string($config->get('unified_multisite_dir'))) {

As per #43.1.

Also, the hook_update should be 8501 and not 8401, since this is version 5 of the module.

Changed patch from #40 according to #46

Maybe I'm missing something here, but there is no other way to modify unified_multisite_dir than hardcoding it into configuration? Is this by design?

Shouldn't it be added in the \Drupal\advagg_mod\Form\SettingsForm? Same goes for the prefetch config property...

[edit]
Looks like it has been removed in c81ea31a530b3f99315917cd774818cdd9517d9c. It's 5 years ago, but maybe @nickdickinsonwilde can recall if it was done on purpose and maybe the leftovers of unified_multisite_dir was forgotten to be removed?

After a review, I believe the last two remaining "unified_multisite_dir" occurrences in the source code should be removed. Then no update hook is needed and the rtrim() case in advagg_mod/src/EventSubscriber/InitSubscriber.php would then be omitted. This could be handled in a separate issue.
In this issue I would then just adjust the stripos() in src/EventSubscriber/ResponseSubscriber.php

Also, I think the MR should be used instead of the patch files.

Hi,

I'm running advagg:^5.0 and we still have Deprecated function : stripos(), have this patch been merged with the lasted version?

Thanks

@sir_squall, no. It's still being worked on..

PHP7.4 is not recommended for D9 sites any more.

Bumping this issue up to Major.

It would be nice to have advagg PHP8.1 compatible release soon.

This is what Status report and D9 + PHP7.4 update.php has started giving now:

Requirements problem
Warnings found
PHP
7.4.33
Your PHP installation is too old. Drupal requires at least PHP 8.0. It is recommended to upgrade to PHP version 8.0 or higher for the best ongoing support. See PHP's version support documentation and the Drupal PHP requirements page for more information.

So, PHP8 compatible releases of contrib modules have become a kind of priority.

I have the same issue in a production site, some update of this ?

I think we need some info from the maintainer regarding #48/#49

@(co)maintainer/s Please add tests against 5.x version.

I also resolved a "code style" error.

If this module is going to be compatible with Drupal 10, PHP 8.1 compliance will be necessary. As soon as there is a test for a ^5 version of the module and PHP 8.1, I could help with this.

This is seeming good on my testing. I'm committing and we can improve more subsequently if need be. Thanks all!

6.0.0 is at alpha1, so not production ready. Is the patch in #57 suitable for 5.0.0?

no need to fear the 6.0.0-alpha1 version - it's 5.0.0 + D10 compatibility fixes + PHP 8.1 compatibility fixes + it gets rid of deprecated sub modules. so if you do not rely on one of them, it's safe to use