Skip to content
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

Prep for PHP 8 #3931

Merged
merged 4 commits into from
Nov 28, 2020
Merged

Prep for PHP 8 #3931

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed5939c
Prep for PHP 8
MGatner Nov 27, 2020
5df9d1a
Backfill misplaced mandatory arguments
MGatner Nov 28, 2020
8af63c5
Allow empty fields
MGatner Nov 28, 2020
b5ef7a0
Restore backfilled parameters (moved to 4.1)
MGatner Nov 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions phpstan.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ parameters:
- system/Commands/Generators/Views/*
- system/Config/Routes.php
- system/Debug/Toolbar/Views/toolbar.tpl.php
- system/Images/Handlers/GDHandler.php
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP 8 uses GDImage class type for the image* functions, which works fine with our current code but breaks the analysis. Ignoring the only file with these references until it can be reworked to support all versions or until PHPStan adds version-specific exemption handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use stub files for this? I'm not sure if there is a capability in phpstan to conditionally include stub files based on the current PHP version.

- system/ThirdParty/*
- system/Validation/Views/single.php
ignoreErrors:
Expand Down
10 changes: 5 additions & 5 deletions system/CodeIgniter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,11 @@ protected function handleRequest(RouteCollectionInterface $routes = null, Cache
{
$controller = $this->createController();

if (! method_exists($controller, '_remap') && ! is_callable([$controller, $this->method], false))
{
throw PageNotFoundException::forMethodNotFound($this->method);
}

// Is there a "post_controller_constructor" event?
Events::trigger('post_controller_constructor');

Expand Down Expand Up @@ -890,11 +895,6 @@ protected function startController()
{
throw PageNotFoundException::forControllerNotFound($this->controller, $this->method);
}
if (! method_exists($this->controller, '_remap') &&
! is_callable([$this->controller, $this->method], false))
Comment on lines -893 to -894
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP 8 changes the behavior of is_callable() such that we need to pass it an instantiated object. Since we are about to get the controller instance anyways I moved this check after.

The ability to call non-static methods statically has been removed. Thus is_callable() will fail when checking for a non-static method with a classname (must check with an object instance).

https://www.php.net/manual/en/migration80.incompatible.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to testing, I think we should change how we invoked phpunit's assert* methods. It is by design a static method and phpunit allows calling it non statically. However when I used phpstan's extra strict rules, I get 'Dynamic calls to static method assert** is not allowed'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're doing it "the PHPUnit way": https://phpunit.readthedocs.io/en/9.3/assertions.html

I didn't even realize they were static. Is there some localized forwarder or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked into the code of TestCase and Assert and didn't find some wrapper to dynamic calls. Maybe phpunit is taking advantage of the way static methods can be called dynamically in php lt 8.

{
throw PageNotFoundException::forMethodNotFound($this->method);
}
}

//--------------------------------------------------------------------
Expand Down
7 changes: 6 additions & 1 deletion system/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,13 @@ class Database
* @return mixed
* @internal param bool $useBuilder
*/
public function load(array $params = [], string $alias)
MGatner marked this conversation as resolved.
Show resolved Hide resolved
public function load(array $params = [], string $alias = '')
{
if (empty($alias))
{
throw new InvalidArgumentException('You must supply the parameter: alias.');
}

// Handle universal DSN connection string
if (! empty($params['DSN']) && strpos($params['DSN'], '://') !== false)
{
Expand Down
2 changes: 1 addition & 1 deletion system/Database/MySQLi/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ public function connect(bool $persistent = false)
if ($this->encrypt['ssl_verify'])
{
defined('MYSQLI_OPT_SSL_VERIFY_SERVER_CERT') &&
$this->mysqli->options(MYSQLI_OPT_SSL_VERIFY_SERVER_CERT, true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another subset of changes are for using booleans for bool|int in places where PHP 8 expects only int.

$this->mysqli->options(MYSQLI_OPT_SSL_VERIFY_SERVER_CERT, 1);
}
// Apparently (when it exists), setting MYSQLI_OPT_SSL_VERIFY_SERVER_CERT
// to FALSE didn't do anything, so PHP 5.6.16 introduced yet another
Expand Down
8 changes: 7 additions & 1 deletion system/HTTP/IncomingRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use CodeIgniter\HTTP\Files\UploadedFile;
use Config\App;
use Config\Services;
use InvalidArgumentException;
use Locale;

/**
Expand Down Expand Up @@ -128,8 +129,13 @@ class IncomingRequest extends Request
* @param string|null $body
* @param UserAgent $userAgent
*/
public function __construct($config, URI $uri = null, $body = 'php://input', UserAgent $userAgent)
public function __construct($config, URI $uri = null, $body = 'php://input', UserAgent $userAgent = null)
{
if (empty($uri) || empty($userAgent))
{
throw new InvalidArgumentException('You must supply the parameters: uri, userAgent.');
}

// Get our body from php://input
if ($body === 'php://input')
{
Expand Down
8 changes: 7 additions & 1 deletion system/Images/Handlers/BaseHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use CodeIgniter\Images\Image;
use CodeIgniter\Images\ImageHandlerInterface;
use Config\Images;
use InvalidArgumentException;

/**
* Base image handling implementation
Expand Down Expand Up @@ -662,8 +663,13 @@ public function fit(int $width, int $height = null, string $position = 'center')
*
* @return array
*/
protected function calcAspectRatio($width, $height = null, $origWidth, $origHeight): array
protected function calcAspectRatio($width, $height = null, $origWidth = 0, $origHeight = 0): array
{
if (empty($origWidth) || empty($origHeight))
{
throw new InvalidArgumentException('You must supply the parameters: origWidth, origHeight.');
}

// If $height is null, then we have it easy.
// Calc based on full image size and be done.
if (is_null($height))
Expand Down
2 changes: 1 addition & 1 deletion system/Log/Handlers/ChromeLoggerHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public function handle($level, $message): bool
$message = $this->format($message);

// Generate Backtrace info
$backtrace = debug_backtrace(false, $this->backtraceLevel);
$backtrace = debug_backtrace(0, $this->backtraceLevel);
$backtrace = end($backtrace);

$backtraceMessage = 'unknown';
Expand Down
2 changes: 1 addition & 1 deletion system/Log/Logger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ public function determineFile(): array
];

// Generate Backtrace info
$trace = \debug_backtrace(false);
$trace = \debug_backtrace(0);

// So we search from the bottom (earliest) of the stack frames
$stackFrames = \array_reverse($trace);
Expand Down
8 changes: 7 additions & 1 deletion system/Router/RouteCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use CodeIgniter\Router\Exceptions\RouterException;
use Config\Modules;
use Config\Services;
use InvalidArgumentException;

/**
* Class RouteCollection
Expand Down Expand Up @@ -988,8 +989,13 @@ public function presenter(string $name, array $options = null): RouteCollectionI
*
* @return RouteCollectionInterface
*/
public function match(array $verbs = [], string $from, $to, array $options = null): RouteCollectionInterface
public function match(array $verbs = [], string $from = '', $to = '', array $options = null): RouteCollectionInterface
{
if (empty($from) || empty($to))
{
throw new InvalidArgumentException('You must supply the parameters: from, to.');
}

foreach ($verbs as $verb)
{
$verb = strtolower($verb);
Expand Down
2 changes: 1 addition & 1 deletion system/Test/ReflectionHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,6 @@ public static function setPrivateProperty($obj, $property, $value)
public static function getPrivateProperty($obj, $property)
{
$refProperty = self::getAccessibleRefProperty($obj, $property);
return $refProperty->getValue($obj);
return is_string($obj) ? $refProperty->getValue() : $refProperty->getValue($obj);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this was working previously, but in PHP 8 ReflectionProperty::getValue() can only take an object parameter. Maybe an undocumented "feature" in 7.x that you could pass a string?

}
}
15 changes: 13 additions & 2 deletions system/Validation/Rules.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace CodeIgniter\Validation;

use Config\Database;
use InvalidArgumentException;

/**
* Validation Rules.
Expand Down Expand Up @@ -357,8 +358,13 @@ public function required($str = null): bool
*
* @return boolean
*/
public function required_with($str = null, string $fields, array $data): bool
public function required_with($str = null, string $fields = '', array $data = []): bool
{
if (empty($fields) || empty($data))
{
throw new InvalidArgumentException('You must supply the parameters: fields, data.');
}

$fields = explode(',', $fields);

// If the field is present we can safely assume that
Expand Down Expand Up @@ -409,8 +415,13 @@ public function required_with($str = null, string $fields, array $data): bool
*
* @return boolean
*/
public function required_without($str = null, string $fields, array $data): bool
public function required_without($str = null, string $fields = '', array $data = []): bool
{
if (empty($fields) || empty($data))
{
throw new InvalidArgumentException('You must supply the parameters: fields, data.');
}

$fields = explode(',', $fields);

// If the field is present we can safely assume that
Expand Down
7 changes: 6 additions & 1 deletion system/Validation/Validation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,13 @@ public function check($value, string $rule, array $errors = []): bool
*
* @return boolean
*/
protected function processRules(string $field, string $label = null, $value, $rules = null, array $data): bool
protected function processRules(string $field, string $label = null, $value, $rules = null, array $data = null): bool
{
if (is_null($data))
MGatner marked this conversation as resolved.
Show resolved Hide resolved
{
throw new InvalidArgumentException('You must supply the parameter: data.');
}

// If the if_exist rule is defined...
if (in_array('if_exist', $rules, true))
{
Expand Down
4 changes: 0 additions & 4 deletions tests/system/Cache/Handlers/FileHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
<?php
namespace CodeIgniter\Cache\Handlers;

set_error_handler(function (int $errno, string $errstr, string $errfile, int $errline, array $errcontext) {
//throw new \ErrorException($errstr, $errno, 0, $errfile, $errline);
});

class FileHandlerTest extends \CodeIgniter\Test\CIUnitTestCase
{

Expand Down
4 changes: 2 additions & 2 deletions tests/system/Helpers/URLHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ public function mailtoPatterns()
/**
* @dataProvider mailtoPatterns
*/
public function testMailto($expected = '', $email, $title = '', $attributes = '')
public function testMailto($expected = '', $email = '', $title = '', $attributes = '')
{
$request = Services::request($this->config);
$request->uri = new URI('http://example.com/');
Expand Down Expand Up @@ -912,7 +912,7 @@ public function safeMailtoPatterns()
/**
* @dataProvider safeMailtoPatterns
*/
public function testSafeMailto($expected = '', $email, $title = '', $attributes = '')
public function testSafeMailto($expected = '', $email = '', $title = '', $attributes = '')
{
$request = Services::request($this->config);
$request->uri = new URI('http://example.com/');
Expand Down