Passing null to parameter #1 ($haystack) of type string is deprecated

Check merge request
In code I have check null values before using in stripos that resolve php 8.1 warning

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

added updated patch and try to fix CCF of MR #14

Think it would worth figuring out what is calling with the empty parameter vs putting the check in. This could be bad code somewhere that should be fixed.

Either way will need a test case.

@smustgrave null is coming from public function get when headers[0] == NULL which is in vendor folder vendor/symfony/http-foundation/HeaderBag.php online 118 how we can change it ad it's not in code base ?

I think smustgrave is right. We need to know why there is a response that is built without any headers. That is pretty weird.

We really need to be able to reproduce this in a way, preferably in a test.

We're seeing this on image generation when adding a webp convert effect to an image style. Agree that there might be an underlying bug about not sending a proper content type in that case, but this seems like something we should just be more defensive about without requiring tests.

I think a simpler fix would be to use $response->headers->get('Content-Type') ?? '' if we can't rely on the default value behaviour in symfony.

> We're seeing this on image generation when adding a webp convert effect to an image style

In our case this was a bug in update function, so having this notice helped identifying that there is a problem in the first place.

For some reasons, this patch is already in Drupal 10.1.x
I upgraded a website using this patch from D9 to D10.1 and the patch was obviously failing.

If issue is no longer a problem in D10.1 we can close out!

Still, the issue exists in D10.1.5

I rerolled patch #19 to work with d10.1.5

I ran into a very similar issue.

\Drupal\jsonapi\Eventsubscriber\ResourceResponseValidator line 104

str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated

$response->headers->all() =

[
  'cache-control' => ['no-cache, private'],
  'date' => ['bla bla'],
  'content-type' => [null],
  'x-drupal-dynamic-cache' => ['UNCACHEABLE'],
];

It would be good to know what causes this problem. "content-type" key should not be there if the value is not a valid mimetype.

Patch #5 took care of three occurrences

1. ActiveLinkResponseFilter
2. RssResponseRelativeUrlFilter
3. ResourceResponseValidator

Patch #29 for an unknown reason does nothing with ResourceResponseValidator.

In our case, we were seeing the 3 deprecation messages when an anonymous user tried to access a Data Export display of a View that required authentication.

The patch in #32 fixes the issue.

This also break contrib tests: e.g. for jsonapi_extras https://www.drupal.org/project/jsonapi_extras/issues/3621534

I have reviewed MR!17048 and read the IS and some of the comments.

Re: #24 it looks like the recommendations have been implemented but tests coverage has also been added though #24 suggests it is unnecessary. Also I ran the test-only test https://git.drupalcode.org/issue/drupal-3344629/-/jobs/12066440 and it does not fail but results instead in a deprecation warning. The warning states that passing in NULL to parameter #1 is now deprecated.

I am changing to Needs work as it might be best to remove the test code especially as one of the test files is edited to fix an unrelated test issue? Not sure whether this issue is outdated since the deprecation landed?

If there is a way of editing the test coverage so it fails on test-only test runs, that could be another possible resolution.

Thanks, that is a fair catch.

#43 Thank you f0ns. I have re-read the IS and I see that the issue itself seems to be the deprecation warning triggered by certain tests. In which case if the test-only test is producing that error that could be what is desired as output from test-only?

I think nothing lost by removing the test coverage, though in line with #24.

Test-only run now fails as requested (job 12070034), since ActiveLinkResponseFilter is in core/lib and gets reverted, with the deprecation promoted to an exception.

#44 The test-only run does now fail, see #45 and job 12070034.

On removing the coverage: #24 was written when this was a deprecation notice. Under declare(strict_types=1) the same NULL is a fatal TypeError, which is what breaks JSON:API Extras (#3621534: phpunit failing in HEAD ). That is why I would keep the test.

Happy to drop the ResourceResponseValidatorTest case if preferred. Its subject sits under */tests/*, so test-only never reverts it and it cannot fail there. The ActiveLinkResponseFilter one does.

The test-only is now producing a test error as desired. RTBTC.

Looking good, +1 for RTBC

Thanks for looking into it and the review, would love to see this move into core.

It would be good to get the bottom of what is setting the header to NULL in these cases?

There doesn't seem to be many places in core that are setting it dynamically

            app/core/modules/jsonapi/src/EventSubscriber  (1 usage found)
                jsonapi/…/ResourceResponseSubscriber.php  (1 usage found)
                    130 $response->headers->set('Content-Type', $request->getMimeType($format));
            app/core/modules/rest/src/EventSubscriber  (1 usage found)
                rest/…/ResourceResponseSubscriber.php  (1 usage found)
                    166 $response->headers->set('Content-Type', $request->getMimeType($format));

    public function getMimeType(string $format): ?string

NULL is a valid return here - so one of those two sound like a culprit

jsonapi_extras.... and Gatsby (uses jsonapi) might point to jsonapi - in tests only?

Request::getMimeType() returns NULL whenever the format isn't in the static Request::$formats array, which Drupal only populates in NegotiationMiddleware::handle().

So the three writers (jsonapi ResourceResponseSubscriber:119, rest ResourceResponseSubscriber:166, serialization DefaultExceptionSubscriber:81) set Content-Type to NULL whenever that middleware didn't run in the process (jsonapi hardcodes api_json) or the route's _format requirement is empty. HeaderBag::set() then stores [null], so has() is TRUE, Response::prepare() can't repair it (both branches are gated on !has('Content-Type')), and get('Content-Type', '') returns NULL because get() explicitly returns NULL before considering the default. Readers have to be NULL-safe either way.

Symfony hit the same thing and applied this exact ?? '' pattern inside Response::prepare() itself. Tightening the writers changes behaviour (no header vs. empty header), so I'd leave that to a follow-up.

Re: #54 That is a very detailed response to #53. Over to @larowlan

Left a comment on the MR

Update is to link to the awesome cmoment in #54.

The open question on the MR feels awnsered.

Committed and pushed 9493974a25a to main and c82f5367255 to 12.0.x and 12981cb0915 to 11.5.x and 89f3e765bb2 to 11.x. Thanks!