Skip to content

fix: phpstan error in v1.8.8#6637

Merged
MGatner merged 3 commits intocodeigniter4:developfrom
ddevsr:patch-1
Oct 6, 2022
Merged

fix: phpstan error in v1.8.8#6637
MGatner merged 3 commits intocodeigniter4:developfrom
ddevsr:patch-1

Conversation

@ddevsr
Copy link
Copy Markdown
Collaborator

@ddevsr ddevsr commented Oct 6, 2022

Description
See https://github.com/codeigniter4/CodeIgniter4/actions/runs/3197626681/jobs/5221091878

Error: Result of && is always false.
Error: Strict comparison using !== between 0 and 0 will always evaluate to false.
 ------ --------------------------------------------------------------------- 
  Line   system/Helpers/array_helper.php                                      
 ------ --------------------------------------------------------------------- 
  51     Result of && is always false.                                        
  51     Strict comparison using !== between 0 and 0 will always evaluate to  
         false.                                                               
 ------ --------------------------------------------------------------------- 

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

@ddevsr ddevsr mentioned this pull request Oct 6, 2022
Copy link
Copy Markdown
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

A poorly timed release!

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Oct 6, 2022

Thanks for the quick response @ddevsr! And the mention + review @datamweb. Waiting on tests.

@ddevsr
Copy link
Copy Markdown
Collaborator Author

ddevsr commented Oct 6, 2022

@MGatner Now PHPUnit & PHPStan already passed

@MGatner
Copy link
Copy Markdown
Member

MGatner commented Oct 6, 2022

The PHPUnit failure looks unrelated (Predis connection failure m). Merging!

@MGatner MGatner merged commit 9b810b8 into codeigniter4:develop Oct 6, 2022
@ddevsr ddevsr deleted the patch-1 branch October 6, 2022 16:36
@kenjis kenjis added the refactor Pull requests that refactor code label Oct 6, 2022
$intCurrentIndex = (int) $currentIndex;

if ((empty($currentIndex) && (int) $currentIndex !== 0) || (! isset($array[$currentIndex]) && $currentIndex !== '*')) {
if (empty($currentIndex) && $intCurrentIndex !== 0 || (! isset($array[$currentIndex]) && $currentIndex !== '*')) {
Copy link
Copy Markdown
Member

@kenjis kenjis Oct 6, 2022

Choose a reason for hiding this comment

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

If $currentIndex is null, empty($currentIndex) is true and $intCurrentIndex !== 0 is false.
If $currentIndex is not null, empty($currentIndex) is false and $intCurrentIndex !== 0 is true.
So the left side is still always false.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this seem better?

        // Grab the current index
        $currentIndex = $indexes ? array_shift($indexes) : null;

        if ($currentIndex === null || (! isset($array[$currentIndex]) && $currentIndex !== '*')) {
            return null;
        }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm sorry, I didn't realize it changed between my review and the merge. This was originally just removing the extraneous part of the conditional.

Yours is better but this still has a lot of cognitive complexity and anyone working on it in the future will need to spend some time figuring it out. Maybe we should break it up into separate return statements? Or extract the second half of the conditional as a variable with a helpful name?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, I will fix this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I sent a PR #6645

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Pull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants