Fixes some code that you'd expect to crash if left in its current state.#41
Open
hostep wants to merge 1 commit intoPayU-EMEA:masterfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We are strongly considering to use this module for a client of ours, usually I run some quick code analysis test (using phpstan and phpcompatibility checker for example) on a module to see if the module is in a good shape or not.
And phpstan found quite some disturbing issues. So here's a PR to fix some of those. I'm not sure if this code actually is being called, but I would expect some of this code to crash if this doesn't get fixed.
Disclaimer: I only changed the code to my best knowledge but didn't test any of this, so please double check all changes and test it to see if it works as expected.
The phpstan errors fixed here are:
I think most of these don't need further explanation, but these 2 are probably good to explain:
GetPostPlaceOrderDatacontroller, the extra$urlprivate member is needed after the change in 0d883fa, where inheritance was changed fromMagento\Framework\App\Action\ActiontoMagento\Framework\App\Action\HttpGetActionInterface, that change looses the protected$_urlmember, so I'm adding it back heremonolog/monologpackage got upgraded to v3 and that marks that class asfinalso try to encourage people not to extend from it, so I extended it fromMagento\Framework\Logger\Monologinstead (yes this is hypocritical, since that class also in its turn extends from theMonolog\Loggerclass, but at least the phpstan warning is gone now), if you disagree with this change, I understand and I can remove it againOther then these errors I fixed, phpstan also found this, which I didn't fix, best is probably to check this yourself, in theory this code can't work without crashes, so maybe this code isn't being used anymore and can be removed?