-
Notifications
You must be signed in to change notification settings - Fork 2k
fix: debug toolbar logs collector behavior on isEmpty()
#9724
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
fix: debug toolbar logs collector behavior on isEmpty()
#9724
Conversation
I'll look into adding a unit test for the Logs controller |
Hi there, mjomble! 👋 Thank you for sending this PR! We expect the following in all Pull Requests (PRs).
Important We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md Sincerely, the mergeable bot 🤖 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change and added test looks good. Thanks for that. However, I'm not sure about adding the default []
to the $data
property. Well, theoretically this is a breaking change as the initial state would now be []
instead of null
. But, practically speaking, the intent was to treat $data
as an array. It's just probably that when this class was created, typed properties were not yet a thing. So, this is a bug fix.
Let's hear from others on their take on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I’m fine with treating changes to the $data
property as a bug fix, though I hope this doesn't open the door to making similar adjustments too easily in the future.
@mjomble Please also add a changelog entry, here: https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.6.4.rst#bugs-fixed PHPStan errors should be gone after merging #9728, so you don't have to worry about it. |
I've updated the changelog |
isEmpty()
thank you @mjomble |
Description
Fixed bug introduced in #9581 and described in this comment
isEmpty()
was checking the opposite$this->data
wasnull
Checklist: