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.

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

refactor: fix phpstan only boolean allowed #9302

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions system/BaseModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ public function findColumn(string $columnName)

$resultSet = $this->doFindColumn($columnName);

return $resultSet ? array_column($resultSet, $columnName) : null;
return $resultSet !== null ? array_column($resultSet, $columnName) : null;
}

/**
Expand Down Expand Up @@ -1137,7 +1137,7 @@ public function delete($id = null, bool $purge = false)
throw new InvalidArgumentException('delete(): argument #1 ($id) should not be boolean.');
}

if ($id && (is_numeric($id) || is_string($id))) {
if (! in_array($id, [null, 0, '0'], true) && (is_numeric($id) || is_string($id))) {
$id = [$id];
}

Expand Down Expand Up @@ -1250,7 +1250,7 @@ public function errors(bool $forceDB = false)
}

// Do we have validation errors?
if (! $forceDB && ! $this->skipValidation && ($errors = $this->validation->getErrors())) {
if (! $forceDB && ! $this->skipValidation && ($errors = $this->validation->getErrors()) !== []) {
return $errors;
}

Expand Down
20 changes: 10 additions & 10 deletions system/CLI/CLI.php
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,12 @@ public static function prompt(string $field, $options = null, $validation = null
$extraOutput = '';
$default = '';

if ($validation && ! is_array($validation) && ! is_string($validation)) {
if (isset($validation) && ! is_array($validation) && ! is_string($validation)) {
throw new InvalidArgumentException('$rules can only be of type string|array');
}

if (! is_array($validation)) {
$validation = $validation ? explode('|', $validation) : [];
$validation = ((string) $validation !== '') ? explode('|', $validation) : [];
Copy link
Member

Choose a reason for hiding this comment

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

Why not check !== null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skip the explode() function with an empty string

Copy link
Member

Choose a reason for hiding this comment

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

That's true, but how many times do we set validation rules without specifying the rules? An empty string will be handled correctly with explode().

Using typing makes the code less clear. I would like to avoid it if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost the same answer #9302 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

An empty string as an edge case, I would not worry about it that much. For me, the readability of the code is more important.

}

if (is_string($options)) {
Expand Down Expand Up @@ -441,7 +441,7 @@ protected static function validate(string $field, string $value, $rules): bool
*/
public static function print(string $text = '', ?string $foreground = null, ?string $background = null)
{
if ($foreground || $background) {
if ((string) $foreground !== '' || (string) $background !== '') {
michalsn marked this conversation as resolved.
Show resolved Hide resolved
$text = static::color($text, $foreground, $background);
}

Expand All @@ -457,7 +457,7 @@ public static function print(string $text = '', ?string $foreground = null, ?str
*/
public static function write(string $text = '', ?string $foreground = null, ?string $background = null)
{
if ($foreground || $background) {
if ((string) $foreground !== '' || (string) $background !== '') {
michalsn marked this conversation as resolved.
Show resolved Hide resolved
$text = static::color($text, $foreground, $background);
}

Expand All @@ -480,7 +480,7 @@ public static function error(string $text, string $foreground = 'light_red', ?st
$stdout = static::$isColored;
static::$isColored = static::hasColorSupport(STDERR);

if ($foreground || $background) {
if ($foreground !== '' || (string) $background !== '') {
michalsn marked this conversation as resolved.
Show resolved Hide resolved
$text = static::color($text, $foreground, $background);
}

Expand Down Expand Up @@ -589,7 +589,7 @@ public static function color(string $text, string $foreground, ?string $backgrou
throw CLIException::forInvalidColor('foreground', $foreground);
}

if ($background !== null && ! array_key_exists($background, static::$background_colors)) {
if ((string) $background !== '' && ! array_key_exists($background, static::$background_colors)) {
michalsn marked this conversation as resolved.
Show resolved Hide resolved
throw CLIException::forInvalidColor('background', $background);
}

Expand Down Expand Up @@ -637,7 +637,7 @@ private static function getColoredText(string $text, string $foreground, ?string
{
$string = "\033[" . static::$foreground_colors[$foreground] . 'm';

if ($background !== null) {
if ((string) $background !== '') {
michalsn marked this conversation as resolved.
Show resolved Hide resolved
$string .= "\033[" . static::$background_colors[$background] . 'm';
}

Expand All @@ -654,7 +654,7 @@ private static function getColoredText(string $text, string $foreground, ?string
*/
public static function strlen(?string $string): int
{
if ($string === null) {
if ((string) $string === '') {
return 0;
}

Expand Down Expand Up @@ -768,7 +768,7 @@ public static function generateDimensions()

// Look for the next lines ending in ": <number>"
// Searching for "Columns:" or "Lines:" will fail on non-English locales
if ($return === 0 && $output && preg_match('/:\s*(\d+)\n[^:]+:\s*(\d+)\n/', implode("\n", $output), $matches)) {
if ($return === 0 && $output !== [] && preg_match('/:\s*(\d+)\n[^:]+:\s*(\d+)\n/', implode("\n", $output), $matches)) {
static::$height = (int) $matches[1];
static::$width = (int) $matches[2];
}
Expand Down Expand Up @@ -835,7 +835,7 @@ public static function showProgress($thisStep = 1, int $totalSteps = 10)
*/
public static function wrap(?string $string = null, int $max = 0, int $padLeft = 0): string
{
if ($string === null || $string === '') {
if ((string) $string === '') {
return '';
}

Expand Down
2 changes: 1 addition & 1 deletion system/Cache/Handlers/BaseHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public static function validateKey($key, $prefix = ''): string
}

$reserved = config(Cache::class)->reservedCharacters ?? self::RESERVED_CHARACTERS;
if ($reserved && strpbrk($key, $reserved) !== false) {
if ($reserved !== '' && strpbrk($key, $reserved) !== false) {
throw new InvalidArgumentException('Cache key contains reserved characters ' . $reserved);
}

Expand Down
3 changes: 2 additions & 1 deletion system/Cache/Handlers/PredisHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Exception;
use Predis\Client;
use Predis\Collection\Iterator\Keyspace;
use Predis\Response\Status;

/**
* Predis cache handler
Expand Down Expand Up @@ -121,7 +122,7 @@ public function save(string $key, $value, int $ttl = 60)
return false;
}

if (! $this->redis->hmset($key, ['__ci_type' => $dataType, '__ci_value' => $value])) {
if (! $this->redis->hmset($key, ['__ci_type' => $dataType, '__ci_value' => $value]) instanceof Status) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion system/Commands/Server/Serve.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public function run(array $params)
// to ensure our environment is set and it simulates basic mod_rewrite.
passthru($php . ' -S ' . $host . ':' . $port . ' -t ' . $docroot . ' ' . $rewrite, $status);

if ($status && $this->portOffset < $this->tries) {
if ($status !== EXIT_SUCCESS && $this->portOffset < $this->tries) {
$this->portOffset++;

$this->run($params);
Expand Down
10 changes: 6 additions & 4 deletions system/Commands/Utilities/Routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public function run(array $params)
$host = $params['host'] ?? null;

// Set HTTP_HOST
if ($host) {
if ($host !== null) {
$request = service('request');
$_SERVER = $request->getServer();
$_SERVER['HTTP_HOST'] = $host;
Expand All @@ -96,7 +96,7 @@ public function run(array $params)
$collection = service('routes')->loadRoutes();

// Reset HTTP_HOST
if ($host) {
if ($host !== null) {
unset($_SERVER['HTTP_HOST']);
}

Expand Down Expand Up @@ -139,7 +139,9 @@ public function run(array $params)
$autoRoutes = $autoRouteCollector->get();

// Check for Module Routes.
if ($routingConfig = config(Routing::class)) {
$routingConfig = config(Routing::class);

if ($routingConfig instanceof Routing) {
foreach ($routingConfig->moduleRoutes as $uri => $namespace) {
$autoRouteCollector = new AutoRouteCollectorImproved(
$namespace,
Expand Down Expand Up @@ -188,7 +190,7 @@ public function run(array $params)
usort($tbody, static fn ($handler1, $handler2) => strcmp($handler1[3], $handler2[3]));
}

if ($host) {
if ($host !== null) {
CLI::write('Host: ' . $host);
}

Expand Down
12 changes: 6 additions & 6 deletions system/Common.php
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ function csrf_hash(): string
*/
function csrf_field(?string $id = null): string
{
return '<input type="hidden"' . ($id !== null ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_token() . '" value="' . csrf_hash() . '"' . _solidus() . '>';
return '<input type="hidden"' . ((string) $id !== '' ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_token() . '" value="' . csrf_hash() . '"' . _solidus() . '>';
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because an empty "" $id doesn't make sense. We skip processing with the esc() function.

Copy link
Member

Choose a reason for hiding this comment

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

How often we will set an empty string as a parameter here? Probably never.

It's already not very readable and with added typing it becomes even less. The world will not end if we escape the empty string.

I admit that this may just be my preference, but using typing everywhere makes the code less clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I've spoken out about rejecting null properties. The check would just be like !== ""
By the way, I noticed for the sake of interest the speed of the check - calling functions for an empty string imposes double the cost 0.02 vs 0.047 Yes, it's a small thing, just so you know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, you can skip the instanceof checks in many places, because checking for null is enough. Right? However, there are strict phpspan and rector rules to ensure that you can work with a valid object. I wanted to introduce stricter typing. Since null seems to be an "archaism from CI3 and PHP < 8"

Copy link
Member

Choose a reason for hiding this comment

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

The "problem" is that an empty string is an edge case, not something we will experience regularly.

But casting to string will be made much more often + readability issues.

In this case, you can skip the instanceof checks in many places, because checking for null is enough. Right? However, there are strict phpspan and rector rules to ensure that you can work with a valid object. I wanted to introduce stricter typing. Since null seems to be an "archaism from CI3 and PHP < 8"

I don't get what you're referring to here. And no, null is not archaism from CI3.

Does PHPStan or Rector have anything against checking that the value is not null? We are talking about specific cases.

Copy link
Contributor Author

@neznaika0 neznaika0 Dec 27, 2024

Choose a reason for hiding this comment

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

Is it possible to allow breaking changes? csrf_field(string $id = '')
ID is an HTML attribute string

}
}

Expand All @@ -301,7 +301,7 @@ function csrf_field(?string $id = null): string
*/
function csrf_meta(?string $id = null): string
{
return '<meta' . ($id !== null ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_header() . '" content="' . csrf_hash() . '"' . _solidus() . '>';
return '<meta' . ((string) $id !== '' ? ' id="' . esc($id, 'attr') . '"' : '') . ' name="' . csrf_header() . '" content="' . csrf_hash() . '"' . _solidus() . '>';
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing this?

}
}

Expand Down Expand Up @@ -441,7 +441,7 @@ function esc($data, string $context = 'html', ?string $encoding = null)
$escaper = new Escaper($encoding);
}

if ($encoding && $escaper->getEncoding() !== $encoding) {
if ((string) $encoding !== '' && $escaper->getEncoding() !== $encoding) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not !== null

$escaper = new Escaper($encoding);
}

Expand Down Expand Up @@ -739,13 +739,13 @@ function lang(string $line, array $args = [], ?string $locale = null)
// Get active locale
$activeLocale = $language->getLocale();

if ($locale && $locale !== $activeLocale) {
if ((string) $locale !== '' && $locale !== $activeLocale) {
$language->setLocale($locale);
}

$lines = $language->getLine($line, $args);

if ($locale && $locale !== $activeLocale) {
if ((string) $locale !== '' && $locale !== $activeLocale) {
// Reset to active locale
$language->setLocale($activeLocale);
}
Expand Down Expand Up @@ -849,7 +849,7 @@ function redirect(?string $route = null): RedirectResponse
{
$response = service('redirectresponse');

if ($route !== null) {
if ((string) $route !== '') {
return $response->route($route);
}

Expand Down
2 changes: 1 addition & 1 deletion system/Database/Forge.php
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ public function dropTable(string $tableName, bool $ifExists = false, bool $casca
return false;
}

if ($this->db->DBPrefix && str_starts_with($tableName, $this->db->DBPrefix)) {
if ($this->db->DBPrefix !== '' && str_starts_with($tableName, $this->db->DBPrefix)) {
$tableName = substr($tableName, strlen($this->db->DBPrefix));
}

Expand Down
16 changes: 8 additions & 8 deletions system/Database/MigrationRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public function latest(?string $group = null)

$this->ensureTable();

if ($group !== null) {
if ((string) $group !== '') {
Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with null?

$this->groupFilter = $group;
$this->setGroup($group);
}
Expand Down Expand Up @@ -326,7 +326,7 @@ public function force(string $path, string $namespace, ?string $group = null)

$this->ensureTable();

if ($group !== null) {
if ((string) $group !== '') {
Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with null?

$this->groupFilter = $group;
$this->setGroup($group);
}
Expand Down Expand Up @@ -389,7 +389,7 @@ public function force(string $path, string $namespace, ?string $group = null)
*/
public function findMigrations(): array
{
$namespaces = $this->namespace ? [$this->namespace] : array_keys(service('autoloader')->getNamespace());
$namespaces = $this->namespace !== null ? [$this->namespace] : array_keys(service('autoloader')->getNamespace());
$migrations = [];

foreach ($namespaces as $namespace) {
Expand Down Expand Up @@ -524,7 +524,7 @@ protected function getMigrationNumber(string $migration): string
{
preg_match($this->regex, $migration, $matches);

return count($matches) ? $matches[1] : '0';
return $matches !== [] ? $matches[1] : '0';
}

/**
Expand All @@ -539,7 +539,7 @@ protected function getMigrationName(string $migration): string
{
preg_match($this->regex, $migration, $matches);

return count($matches) ? $matches[2] : '';
return $matches !== [] ? $matches[2] : '';
}

/**
Expand Down Expand Up @@ -645,7 +645,7 @@ public function getHistory(string $group = 'default'): array
}

// If a namespace was specified then use it
if ($this->namespace) {
if ($this->namespace !== null) {
$builder->where('namespace', $this->namespace);
}

Expand Down Expand Up @@ -700,7 +700,7 @@ public function getLastBatch(): int
->get()
->getResultObject();

$batch = is_array($batch) && count($batch)
$batch = is_array($batch) && $batch !== []
? end($batch)->batch
: 0;

Expand All @@ -725,7 +725,7 @@ public function getBatchStart(int $batch): string
->get()
->getResultObject();

return count($migration) ? $migration[0]->version : '0';
return $migration !== [] ? $migration[0]->version : '0';
}

/**
Expand Down
8 changes: 5 additions & 3 deletions system/Database/MySQLi/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public function connect(bool $persistent = false)
$clientFlags
)) {
// Prior to version 5.7.3, MySQL silently downgrades to an unencrypted connection if SSL setup fails
if (($clientFlags & MYSQLI_CLIENT_SSL) && version_compare($this->mysqli->client_info, 'mysqlnd 5.7.3', '<=')
if (($clientFlags & MYSQLI_CLIENT_SSL) !== 0 && version_compare($this->mysqli->client_info, 'mysqlnd 5.7.3', '<=')
&& empty($this->mysqli->query("SHOW STATUS LIKE 'ssl_cipher'")->fetch_object()->Value)
) {
$this->mysqli->close();
Expand Down Expand Up @@ -395,7 +395,7 @@ protected function _listTables(bool $prefixLimit = false, ?string $tableName = n
{
$sql = 'SHOW TABLES FROM ' . $this->escapeIdentifier($this->database);

if ($tableName !== null) {
if ((string) $tableName !== '') {
return $sql . ' LIKE ' . $this->escape($tableName);
}

Expand Down Expand Up @@ -462,7 +462,9 @@ protected function _indexData(string $table): array
throw new DatabaseException(lang('Database.failGetIndexData'));
}

if (! $indexes = $query->getResultArray()) {
$indexes = $query->getResultArray();

if ($indexes === []) {
return [];
}

Expand Down
4 changes: 2 additions & 2 deletions system/Database/SQLite3/Connection.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public function connect(bool $persistent = false)
$this->database = WRITEPATH . $this->database;
}

$sqlite = (! $this->password)
$sqlite = (! isset($this->password) || $this->password !== '')
? new SQLite3($this->database)
: new SQLite3($this->database, SQLITE3_OPEN_READWRITE | SQLITE3_OPEN_CREATE, $this->password);

Expand Down Expand Up @@ -194,7 +194,7 @@ protected function _escapeString(string $str): string
*/
protected function _listTables(bool $prefixLimit = false, ?string $tableName = null): string
{
if ($tableName !== null) {
if ((string) $tableName !== '') {
return 'SELECT "NAME" FROM "SQLITE_MASTER" WHERE "TYPE" = \'table\''
. ' AND "NAME" NOT LIKE \'sqlite!_%\' ESCAPE \'!\''
. ' AND "NAME" LIKE ' . $this->escape($tableName);
Expand Down
6 changes: 4 additions & 2 deletions system/Email/Email.php
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ public function setFrom($from, $name = '', $returnPath = null)
if ($this->validate) {
$this->validateEmail($this->stringToArray($from));

if ($returnPath) {
if ($returnPath !== null) {
$this->validateEmail($this->stringToArray($returnPath));
}
}
Expand Down Expand Up @@ -1233,7 +1233,9 @@ protected function buildMessage()
$this->headerStr .= $hdr;
}

static::strlen($body) && $body .= $this->newline . $this->newline;
if (static::strlen($body) > 0) {
$body .= $this->newline . $this->newline;
}

$body .= $this->getMimeMessage() . $this->newline . $this->newline
. '--' . $lastBoundary . $this->newline
Expand Down
Loading
Loading