-
Notifications
You must be signed in to change notification settings - Fork 14
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
IBX-7055: Allowed to return null
when content is not deleted
#297
Conversation
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.
Is this needed for some functionality?
In theory here, if there is a chance that result is not set, then BeforeTrashEvent::hasTrashItem()
method should be called first. Because of how BeforeTrashEvent::getResult()
works internally, null
will never be returned - an exception will be thrown.
What you want to do instead is probably add phpstan-assert-if-true
annotation to BeforeTrashEvent::isResultSet()
method, instructing PHPStan that when this method returns true
, $this->result
will not be null
.
See https://phpstan.org/writing-php-code/narrowing-types#custom-type-checking-functions-and-methods for more details.
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.
At least unit test layer for events should include coverage for this change.
308d0bf
to
3710f3c
Compare
Running regressions via ibexa/commerce#508 |
3710f3c
to
4e73c46
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Solr failure is unrelated, regressions all green. The change is too trivial for QA, merging :) |
v4.6
When content is not deleted (e.g. on before event propagation is stopped) the
null
value must be set as a result.Checklist:
$ composer fix-cs
).@ibexa/engineering
).