Skip to content

[HttpFoundation] PHP 8.1 deprecation notice in IpUtils::checkIp() #43350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
W0rma opened this issue Oct 6, 2021 · 4 comments
Closed

[HttpFoundation] PHP 8.1 deprecation notice in IpUtils::checkIp() #43350

W0rma opened this issue Oct 6, 2021 · 4 comments
Labels
Bug HttpFoundation Status: Needs Review

Comments

@W0rma
Copy link
Contributor

W0rma commented Oct 6, 2021 •
edited
Loading

Symfony version(s) affected: 5.3

Description
I stumbled upon the method IpUtils::checkIp():

    /**
     * Checks if an IPv4 or IPv6 address is contained in the list of given IPs or subnets.
     *
     * @param string|array $ips List of IPs or subnets (can be a string if only a single one)
     *
     * @return bool Whether the IP is valid
     */
    public static function checkIp(?string $requestIp, $ips)
    {
        if (!\is_array($ips)) {
            $ips = [$ips];
        }

        $method = substr_count($requestIp, ':') > 1 ? 'checkIp6' : 'checkIp4';

        foreach ($ips as $ip) {
            if (self::$method($requestIp, $ip)) {
                return true;
            }
        }

        return false;
    }

substr_count() is called even if $requestIp === null which causes a warning in PHP 8.1 ("substr_count(): Passing null to parameter #1 ($haystack) of type string is deprecated").

How to reproduce

Possible Solution
I guess we could just avoid calling substr_count() if the request ip is null.

Additional context
@W0rma W0rma added the Bug label Oct 6, 2021
@W0rma W0rma changed the title [HttpFoundation] Wrong typehint in IpUtils::checkIp() [HttpFoundation] PHP 8.1 warning in IpUtils::checkIp() Oct 6, 2021
@W0rma
Copy link
Contributor Author

W0rma commented Oct 6, 2021

Related to #41552

@W0rma W0rma changed the title [HttpFoundation] PHP 8.1 warning in IpUtils::checkIp() [HttpFoundation] PHP 8.1 deprecation notice in IpUtils::checkIp() Oct 6, 2021
@stof
Copy link
Member

stof commented Oct 6, 2021

We should also deprecate passing null IMO. Before #33088, it was documented as @param string $ip

@derrabus
Copy link
Member

derrabus commented Oct 6, 2021

Would you like to contribute a fix, @W0rma?

@W0rma W0rma mentioned this issue Oct 7, 2021
Merged
@W0rma
Copy link
Contributor Author

W0rma commented Oct 7, 2021

Sure, I've just created #43357 to fix the deprecation warning and I will create additional PRs to deprecate/remove the possibility to pass null to various methods of IpUtils.

fabpot added a commit that referenced this issue Oct 10, 2021
@fabpot
minor #43357 [HttpFoundation] Fix PHP 8.1 deprecation notice in IpUti…
ab7884c 
…ls::checkIp() (W0rma)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] Fix PHP 8.1 deprecation notice in IpUtils::checkIp()

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #43350
| License       | MIT
| Doc PR        |

After this PR got merged I will create additional PRs to deprecate (5.4)/remove (6.0) passing `null` to various methods in `IpUtils` like suggested in #43350 (comment)

Commits
-------

5e991db Do not call substr_count() if ip is null to avoid deprecation warning in PHP 8.1
@fabpot fabpot closed this as completed Oct 10, 2021
@W0rma W0rma mentioned this issue Oct 12, 2021
Merged
fabpot added a commit that referenced this issue Oct 16, 2021
@fabpot
feature #43411 [HttpFoundation] Deprecate passing null as $requestIp …
a53a173 
…in IpUtils (W0rma)

This PR was merged into the 5.4 branch.

Discussion
----------

[HttpFoundation] Deprecate passing null as $requestIp in IpUtils

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | #43350 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

55e3a5b Deprecate passing null as $requestIp to IpUtils::checkIp(), checkIp4() and checkIp6()
@W0rma W0rma mentioned this issue Oct 17, 2021
Merged
fabpot added a commit that referenced this issue Oct 18, 2021
@fabpot
feature #43550 [HttpFoundation] Remove possibility to pass null as $r…
be8401b 
…equestIp in IpUtils (W0rma)

This PR was merged into the 6.0 branch.

Discussion
----------

[HttpFoundation] Remove possibility to pass null as $requestIp in IpUtils

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #43350 (comment)
| License       | MIT
| Doc PR        |

Removes the code which was deprecated in #43411

Commits
-------

e95e97d Remove possibility to pass null as $requestIp in IpUtils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug HttpFoundation Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @stof @derrabus @carsonbot @W0rma