refactor: replace empty() Part 1 #8345

kenjis · 2023-12-18T07:12:55Z

~~Needs #8344~~

Description

replace empty()

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

github-actions · 2023-12-21T22:19:28Z

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

github-actions · 2023-12-25T22:01:20Z

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

michalsn · 2023-12-28T07:27:50Z

system/Model.php

@@ -257,13 +257,13 @@ protected function doFirst()

        if ($this->tempUseSoftDeletes) {
            $builder->where($this->table . '.' . $this->deletedField, null);
-        } elseif ($this->useSoftDeletes && empty($builder->QBGroupBy) && $this->primaryKey) {
+        } elseif ($this->useSoftDeletes && ($builder->QBGroupBy === []) && $this->primaryKey) {


Is () around $builder->QBGroupBy === [] is forced by our tooling? Seems unnecessary.

michalsn · 2023-12-28T07:28:07Z

system/Model.php

            $builder->groupBy($this->table . '.' . $this->primaryKey);
        }

        // Some databases, like PostgreSQL, need order
        // information to consistently return correct results.
-        if ($builder->QBGroupBy && empty($builder->QBOrderBy) && $this->primaryKey) {
+        if ($builder->QBGroupBy && ($builder->QBOrderBy === []) && $this->primaryKey) {


Same as above.

michalsn · 2023-12-28T07:30:32Z

system/Test/FeatureTestCase.php

@@ -186,7 +186,7 @@ public function call(string $method, string $path, ?array $params = null)
            ->run($routes, true);

        $output = \ob_get_contents();
-        if (empty($response->getBody()) && ! ($output === '' || $output === false)) {
+        if (($response->getBody() === null) && ! ($output === '' || $output === false)) {


Probably unnecessary () around $response->getBody() === null.

michalsn · 2023-12-28T07:33:09Z

system/Validation/Validation.php

@@ -918,7 +918,7 @@ protected function getErrorMessage(
        $message = str_replace('{field}', ($label === null || $label === '') ? $field : lang($label), $message);
        $message = str_replace(
            '{param}',
-            empty($this->rules[$param]['label']) ? $param : lang($this->rules[$param]['label']),
+            (! isset($this->rules[$param]['label'])) ? $param : lang($this->rules[$param]['label']),


Do we need () around ! isset($this->rules[$param]['label'])?

michalsn · 2023-12-28T07:34:32Z

system/View/Parser.php

@@ -103,7 +103,7 @@ public function render(string $view, ?array $options = null, ?bool $saveData = n
        }

        $fileExt = pathinfo($view, PATHINFO_EXTENSION);
-        $view    = empty($fileExt) ? $view . '.php' : $view; // allow Views as .html, .tpl, etc (from CI3)
+        $view    = ($fileExt === '') ? $view . '.php' : $view; // allow Views as .html, .tpl, etc (from CI3)


Same here... is it supposed to increase readability? For me, it's the opposite.

In this case, yes.

michalsn · 2023-12-28T07:36:17Z

system/View/Parser.php

@@ -589,7 +589,7 @@ protected function applyFilters(string $replace, array $filters): string
            preg_match('/\([\w<>=\/\\\,:.\-\s\+]+\)/u', $filter, $param);

            // Remove the () and spaces to we have just the parameter left
-            $param = ! empty($param) ? trim($param[0], '() ') : null;
+            $param = ($param !== []) ? trim($param[0], '() ') : null;


With only one check, I see no added value in using ().

michalsn · 2023-12-28T07:37:14Z

system/View/View.php

@@ -178,7 +178,7 @@ public function render(string $view, ?array $options = null, ?bool $saveData = n

        $fileExt = pathinfo($view, PATHINFO_EXTENSION);
        // allow Views as .html, .tpl, etc (from CI3)
-        $this->renderVars['view'] = empty($fileExt) ? $view . '.php' : $view;
+        $this->renderVars['view'] = ($fileExt === '') ? $view . '.php' : $view;


michalsn · 2023-12-28T07:37:25Z

system/View/View.php

@@ -207,12 +207,12 @@ public function render(string $view, ?array $options = null, ?bool $saveData = n
            $this->renderVars['file'] = $this->loader->locateFile(
                $this->renderVars['view'],
                'Views',
-                empty($fileExt) ? 'php' : $fileExt
+                ($fileExt === '') ? 'php' : $fileExt


michalsn

I left a few comments, but it may be only my preference.

kenjis · 2023-12-28T07:58:24Z

There are some unnecessary (), but I merge this for now.

kenjis marked this pull request as draft December 18, 2023 07:13

kenjis force-pushed the replace-empty branch 4 times, most recently from 102b20c to c666bf7 Compare December 19, 2023 03:04

github-actions bot added the stale Pull requests with conflicts label Dec 21, 2023

kenjis force-pushed the replace-empty branch from 52a6dbc to bd0ef26 Compare December 21, 2023 23:51

kenjis added refactor Pull requests that refactor code and removed stale Pull requests with conflicts labels Dec 21, 2023

kenjis force-pushed the replace-empty branch from 0364948 to 687b611 Compare December 22, 2023 01:46

kenjis changed the title ~~refactor: replace empty()~~ refactor: replace empty() Part 1 Dec 22, 2023

kenjis marked this pull request as ready for review December 22, 2023 10:58

github-actions bot added the stale Pull requests with conflicts label Dec 25, 2023

kenjis added 10 commits December 26, 2023 07:24

refactor: replace empty()

d510a42

refactor: replace empty()

d758774

refactor: replace empty()

31c5c14

refactor: replace empty()

0ece99b

chore: add --exclude for phpcpd

8d17de2

chore: remove ignoreErrors

424444f

refactor: replace empty()

8254eb7

chore: remove ignoreErrors

479b35d

refactor: replace empty()

69fbb6c

chore: remove ignoreErrors

269986e

kenjis force-pushed the replace-empty branch from 1e92f25 to 269986e Compare December 25, 2023 22:27

kenjis removed the stale Pull requests with conflicts label Dec 25, 2023

michalsn reviewed Dec 28, 2023

View reviewed changes

michalsn approved these changes Dec 28, 2023

View reviewed changes

kenjis merged commit 838900e into codeigniter4:develop Dec 28, 2023
61 checks passed

kenjis deleted the replace-empty branch December 28, 2023 07:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace empty() Part 1 #8345

refactor: replace empty() Part 1 #8345

kenjis commented Dec 18, 2023 •

edited

Loading

github-actions bot commented Dec 21, 2023

github-actions bot commented Dec 25, 2023

michalsn Dec 28, 2023

michalsn Dec 28, 2023

michalsn Dec 28, 2023

michalsn Dec 28, 2023

michalsn Dec 28, 2023

kenjis Dec 28, 2023

michalsn Dec 28, 2023

michalsn Dec 28, 2023

michalsn Dec 28, 2023

michalsn left a comment

kenjis commented Dec 28, 2023

refactor: replace empty() Part 1 #8345

refactor: replace empty() Part 1 #8345

Conversation

kenjis commented Dec 18, 2023 • edited Loading

github-actions bot commented Dec 21, 2023

github-actions bot commented Dec 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalsn left a comment

Choose a reason for hiding this comment

kenjis commented Dec 28, 2023

kenjis commented Dec 18, 2023 •

edited

Loading