Bug: Incorrect type that fails strict_types=1

[John-Betong]

<?php declare(strict_types=1); ?>

I have prefixed every system PHP file with the above script and encountered the following two anomalies:

// INCORRECT SCRIPT
./system/HTTP/RequestInterface.php -> public function isValidIP(string $ip, string $which = null): bool;

// CORRECT USAGE
./system/HTTP/RequestInterface.php -> public function isValidIP(string $ip, string $which = ''): bool;

It is worth noting that I think it is advisable to also change the following to blank strings instead of NULL
// INVALID STRING NULL
./system/HTTP/Request.php -> public function isValidIP(string $ip = null, string $which = null): bool

// VALID ACCORDING TO PHP Manual
./system/HTTP/Request.php -> public function isValidIP(string $ip = '', string $which = ''): bool

// ERRORS THROWN DUE TO PHP MANUAL expecting strings and NULL is not a string
https://www.php.net/manual/en/function.filter-var.php

CodeIgniter 4 version
https://github.com/codeigniter4/CodeIgniter4.git
Version - 2020-09-05 - fe016d9

Expected behavior, and steps to reproduce if appropriate
Fatal error: Declaration of CodeIgniter\HTTP\Request::isValidIP(string $ip = '', string $which = ''): bool must be compatible with CodeIgniter\HTTP\RequestInterface::isValidIP(string $ip, ?string $which = NULL): bool in /var/www/ci4-strict.tk/system/HTTP/Request.php on line 240

Context

• OS: Linux Ubuntu 20.04
• Web server Apache 2
• PHP version 7.4.9

[MGatner]

I think these are both good changes. Could you submit a PR with these so we can review and merge?

[michalsn]

Personally I would rather stay with null as default since this is what we use in general for all method variables.

Are you sure this is about filter_var()? Because I'm quite sure that null is a valid value for the 3rd parameter. Looking at the code I would rather say that it's about strtolower($which) - this should be cast to string and that's all.

[John-Betong]

@michalsn,

I have just downloaded the latest 2020-09-06 - Build:473a8f5 and prefixed "Strict_types=1" into 450 PHP files.

The errors are eliminated when the two suggested modifications are made in accordance with the PHP Manual.

I believe that NULL should not be used because PHP 8.0 is imminent and conforming to strict_types will most probably be essential when trying to apply Just in Time compilation.

Perhaps a thread could be raised and hopefully a knowlegeable user will be able to suggest a valid reason rather than speculation.

Regarding "Personally I would rather stay with null as default... " - may I suggest adding strict_types to your source files. I was amazed to discover that "ini_set('display_errors', 1)" and "ini_set('display_errors', TRUE)" both raised a errors!!!

[michalsn]

I'm not claiming you're wrong, but maybe you could try to change only this line strtolower($which) (cast variable to string) and we will accomplish the same result (no errors)? if that won't help, then I'm good with the proposed changes.

[John-Betong]

@michalsn ,

I just tried your suggestion and delighted to say it works.

There are now two solutions and I do think your solution is a lot simpler.

Which solution to raise a PR?

[michalsn]

Thanks for checking it out. Personally, I always think simpler is better.