Skip to content

refactor: fix various phpstan errors in Log component - #9581

Merged
michalsn merged 10 commits into
codeigniter4:developfrom
paulbalandan:refactor-logger
May 31, 2025
Merged

refactor: fix various phpstan errors in Log component#9581
michalsn merged 10 commits into
codeigniter4:developfrom
paulbalandan:refactor-logger

Conversation

@paulbalandan

Copy link
Copy Markdown
Member

Description
Fixing some phpstan errors. Hope I did not break anything.

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

@paulbalandanpaulbalandan added the refactor Pull requests that refactor code label May 26, 2025
Comment threadtests/system/Log/LoggerTest.php
Comment threadsystem/Debug/Toolbar/Collectors/Logs.php Outdated
Comment threadsystem/Log/Handlers/FileHandler.php Outdated
Comment threadsystem/Log/Logger.php Outdated
Comment threadsystem/Log/Logger.php Outdated

@michalsnmichalsn left a comment

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.

Thanks, looks good!

@michalsn
michalsn merged commit 63ab555 into codeigniter4:developMay 31, 2025
@paulbalandan
paulbalandan deleted the refactor-logger branch May 31, 2025 08:01
@mjomble

Copy link
Copy Markdown
Contributor

Looks like it did break something 😁

In isEmpty(), return empty($this->data); was correct, but return $this->data !== []; actually checks for "is not empty".

Also, both this and if (! empty($this->data)) { in collectLogs() assume $this->data is always an array, but it starts out as null.

And ironically, due to this very assumption, it will always remain null 😄

Seems to me like empty() is perfect here, covering both the null and empty array cases.
Does phpstan recommend avoiding it? If so, I wonder why?

@paulbalandan

Copy link
Copy Markdown
MemberAuthor

Hi @mjomble , please send a PR fixing those. We're moving away from use of empty() as it is a loose check.

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

Labels

refactorPull requests that refactor code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@paulbalandan@mjomble@michalsn