Extract method refactoring for ResourceCheckerConfigCache #22589
Conversation
| throw $e; | ||
| } | ||
| if (false === $meta) { | ||
| return false; |
There was a problem hiding this comment.
I think this should become null instead of false, the way the method is used, it looks like a nullable return type.
There was a problem hiding this comment.
the return type is mixed, so nullable or not doesn't matter really.
Still, this "if" should be just removed, the return $meta; below is enough here.
There was a problem hiding this comment.
Good catch! I just extracted the method without additional scrutinity and indeed these lines are no longer needed.
|
This class exist since 2.8. Shouldn't this refactoring (if it's worth) be done in this branch? |
|
I think refactoring should be done on master. Maintenance branches should be updated for bug fixes only, avoiding behavior changes and silly BC breaks on patch releases (potentially breaking apps expecting to be safe by locking on a LTS, and that for no strong reason). |
|
@chalasr ... which is exactly how this is handled. |
|
@mpdude sure, just to give one more point of view :) this looks good to me. |
|
@chalasr the problem of refactoring in the master branch is that it increases the number of merge conflicts when merging other PRs from lower branches to higher. |
|
Sure. Question is ... do we prefer merge conflicts potentially leading to bad merges on our dev branch (internal and, most of the times, quickly fixed) or potential breakages on production (userland, stable stuff suddenly broken with generally 1month for being fixed) |
|
It's not refactoring really.. just moving code around :) i would do this on 2.8 to avoid the merge conflicts. But is it worth it? 🤔 we only change a single occurrence; i.e. what are you solving? |
|
Improving clarity, always worth it. |
|
@fabot What do you think – should such refactorings go into the oldest branch to avoid merge conflicts later on, or are changes on old branches strictly bugfix-only? |
|
Doing this in 3.4 looks good to me. |
|
Thank you @mpdude. |
…e (mpdude) This PR was squashed before being merged into the 3.4 branch (closes #22589). Discussion ---------- Extract method refactoring for ResourceCheckerConfigCache | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Improves code readability. Commits ------- 685c353 Extract method refactoring for ResourceCheckerConfigCache
Improves code readability.