Fix PHP deprecation errors - Issue #13270

[markusVJH]

Q	A
Bug fix? (use the a.b branch)	[ x ]
New feature/enhancement? (use the a.x branch)	[ ]
Deprecations?	[ ]
BC breaks? (use the c.x branch)	[ ]
Automated tests included?	[ ]
Related user documentation PR URL	mautic/mautic-documentation#...
Related developer documentation PR URL	mautic/developer-documentation#...
Issue(s) addressed	Fixes #13270

Description:

When visiting the login page, the logs could have these deprecation notices:

PHP Deprecation - fnmatch(): Passing null to parameter #2 ($filename) of type string is deprecated - in file /app/middlewares/CORSMiddleware.php - at line 107
PHP Deprecated:  fnmatch(): Passing null to parameter #2 ($filename) of type string is deprecated in /app/middlewares/CORSMiddleware.php on line 107

This PR implements a simple fix proposed by @J-Wick4, where the variable $origin (which is passed as parameter 2 mentioned in the error message) is checked whether it is null before calling fnmatch(). If $origin is null, false is returned, and the function continues as before. This fixes the error messages.

Steps to test this PR:

1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
2. Go to the login page.
3. Refresh the page, and check that the logs do not have the afore mentioned errors.

If testing locally with docker, you can keep eyes on the logs through Docker.

If testing on GitPod through browser, you can check the 'Problems' tab to see if the deprecated notice is there (expected string, found null).

If testing with GitPod but not through browser, you can run ddev logs in the ide terminal to see the logs.

If you want to first see the error messages, and then see the effect of the solution:

1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

2. Replace function getAllowOriginHeaderValue(Request $request) with the following version. It makes sure the $origin is null and that the fnmatch is called in order to replicate the situation people were having with the deprecation notices in logs. in CORSMiddleware.php:

    private function getAllowOriginHeaderValue(Request $request)
    {
        // Temporary conditions for testing issue #13270
        $origin = null;
        $this->restrictCORSDomains = true; // Ensure domain restriction is enforced
        $this->validCORSDomains = ['*']; // Assuming a wildcard for simplicity

        // If we're not restricting domains, set the header to the request origin
        if (!$this->restrictCORSDomains || in_array($origin, $this->validCORSDomains)) {
            $this->requestOriginIsValid = true;

            return $origin;
        }

        // Check the domains using shell wildcard patterns
        $validCorsDomainFilter = function ($validCorsDomain) use ($origin) {
            return fnmatch($validCorsDomain, $origin, FNM_CASEFOLD);
        };
        if (array_filter($this->validCORSDomains, $validCorsDomainFilter)) {
            $this->requestOriginIsValid = true;
            $this->corsHeaders['Vary']  = 'Origin';

            return $origin;
        }

        $this->requestOriginIsValid = false;

        return null;
    }

3. Add the previous test code, go to the log in page, and refresh the page. Now the error messages should show in the logs (Either the 'Problems' tab in the GitPod browser, ddev logs result in ide terminal or logs through Docker).

4. Then to see the error messages fixed with the new solution, add the following code, refresh the log in page and check that the deprecation notice no longer appears:

    private function getAllowOriginHeaderValue(Request $request)
    {
        // Temporary conditions for testing issue #13270
        $origin = null;
        $this->restrictCORSDomains = true; // Ensure domain restriction is enforced
        $this->validCORSDomains = ['*']; // Assuming a wildcard for simplicity

        // If we're not restricting domains, set the header to the request origin
        if (!$this->restrictCORSDomains || in_array($origin, $this->validCORSDomains)) {
            $this->requestOriginIsValid = true;

            return $origin;
        }

        // Check the domains using shell wildcard patterns
        $validCorsDomainFilter = function ($validCorsDomain) use ($origin) {
            if ($origin === null) {
                return false;
            }

            return fnmatch($validCorsDomain, $origin, FNM_CASEFOLD);
        };

        if (array_filter($this->validCORSDomains, $validCorsDomainFilter)) {
            $this->requestOriginIsValid = true;
            $this->corsHeaders['Vary']  = 'Origin';

            return $origin;
        }

        $this->requestOriginIsValid = false;

        return null;
    }

[RCheesley]

Thanks for the PR @markusVJH and welcome to the community! I've just set the tests running 🚀

[RCheesley]

@markusVJH could you check out the failing tests please, let us know if you need any help!

[markusVJH]

Thanks for the PR @markusVJH and welcome to the community! I've just set the tests running 🚀

Thank you! I will check the failing tests today.

[markusVJH]

@markusVJH could you check out the failing tests please, let us know if you need any help!

Added the change for the CS Fixer job 👍 For the codecov test I am not sure what to do. Based on Slack messages I found it looks like re-running the job might fix it?

[RCheesley]

Thanks @markusVJH - we have had some issues with Codecov as they deprecated an older version which we were running in favour of the GitHub app - should be all fixed now I've merged in the latest changes.

Would you be able to rebase this onto the 5.x branch if possible? We're about to release 5.1 so we should be able to get this into that, or the next patch release.

[markusVJH]

Thank you for the reply, I did later find that a lot of PRs were having the same issue 🙏

I will try to do the rebase 👍

[markusVJH]

Thanks @markusVJH - we have had some issues with Codecov as they deprecated an older version which we were running in favour of the GitHub app - should be all fixed now I've merged in the latest changes.

Would you be able to rebase this onto the 5.x branch if possible? We're about to release 5.1 so we should be able to get this into that, or the next patch release.

Rebased to 5.x! Thank you @escopecz for the advice

Hopefully the tests will pass

[escopecz]

The tests don't want to kick off. I'll close and reopen the PR. It usually fixes it.

[escopecz]

Thanks! The code change looks good to me 👍

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 61.50%. Comparing base (b2345c1) to head (99b7f41).

Additional details and impacted files

@@             Coverage Diff              @@
##                5.x   #13615      +/-   ##
============================================
- Coverage     61.50%   61.50%   -0.01%     
- Complexity    34067    34068       +1     
============================================
  Files          2241     2241              
  Lines        101850   101852       +2     
============================================
  Hits          62646    62646              
- Misses        39204    39206       +2     

Files	Coverage Δ
app/middlewares/CORSMiddleware.php	0.00% <0.00%> (ø)

[RCheesley]

Hi @markusVJH I am trying to reproduce this before testing the PR but I am struggling - neither locally with DDEV or on Gitpod can I see those errors.

I see plenty of other deprecation warnings in my DDEV logs, but not this one.

Any chance it was fixed somewhere else?

[markusVJH]

Hi @RCheesley ! Sorry the late reply, I was away :s

We had this error showing up in the logs after a new Mautic installation for production. I was asked if I wanted to try to make a pr that would fix it from happening.

I struggled to repeat the error locally, but I added that testing guidance where $origin can be forced to be null in order test the errors coming up like we had in the production, and then test how the fix would remove them even if $origin were to be null. Is this ok?

[RCheesley]

If @escopecz thinks it's a good addition then it's fine with me - I just wasn't able to do the user testing as I couldn't reproduce the bug in the first place ;)

[escopecz]

This cannot hurt anything. It happens only if the Origin header in the request is missing.

My previous suggestion to change return false to return null was wrong as it's in a callback function, not the return of the method itself. But null is evaluated by PHP as false so it has no functional effect. So let's keep it as is.