-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cleanup dead setDefaultOptions(..) methods #6334
Conversation
Codecov Report
@@ Coverage Diff @@
## 3.x #6334 +/- ##
======================================
Coverage ? 77.59%
Complexity ? 2600
======================================
Files ? 140
Lines ? 7743
Branches ? 0
======================================
Hits ? 6008
Misses ? 1735
Partials ? 0 Continue to review full report at Codecov.
|
Removing a public method is always a BC-break. |
Can you explain how this could be a BC break? In which scenario would this break anything? |
Someone really crazy could have written in his code
It's not because symfony doesn't use it anymore that we can be sure that no one use it. |
Doing a |
Oh ! Ok, I understand. |
Technicly it is BC-break. AdminBundle require BlockBundle 3.x or 4.x. In 3.x in It is not ower namespace and it is not support for Symfony so remove it is BC. IMO we should remove it in BlockBundle 3.x too. We should not using not ower namespaces. @dmaicher Can you do it for BlockBundle too? https://github.com/sonata-project/SonataBlockBundle/blob/3.x/src/Block/Service/AbstractBlockService.php#L135-L144 |
Oh wow. That is an interesting "hack" on the BlockBundle. Unfortunately there we cannot remove it yet I think. There is this class: https://github.com/sonata-project/SonataBlockBundle/blob/3.x/src/Util/OptionsResolver.php Which is used on the bundle itself and will use the stub interface |
@dmaicher |
@wbloszyk but the instance passed to those https://github.com/sonata-project/SonataBlockBundle/blob/3.x/src/Block/BlockContextManager.php#L240 So the methods are still called. Which means removing them would break BC for sure. |
It is strange hack which is remove in 4.x. I think it is OK. @dmaicher Thanks for check it. |
Thanks @dmaicher |
Subject
While checking some of the phpstan errors on this PR I noticed some related to the non-existing
OptionsResolverInterface
.So I propose here to cleanup this dead code to simplify maintenance a bit 😊
I don't see any way how this could be a BC break for anyone.
Those methods are not part of any contract and cannot be called anymore since
Symfony\Component\OptionsResolver\OptionsResolverInterface
does not even exist.Or did I miss something?
Actually @franmomu also suggested to remove them here.
I am targeting this branch, because I believe this is BC.
Changelog