-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Replaced ArrayInterface with OptionSourceInterface #19308
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
Conversation
Hi @sreichel. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
Considering that theoretically, this is backward incompatible change, probably it would be better to mark the interface as deprecated first. However, I think it is very unlikely that ArrayInterface is used as a parameter type anywhere. Added this pull request to discuss at a group code review. |
@sivaschenko Thanks for reply, I've not removed the interface. It marked as deprecated and not used in core. Should not break anything. (?) |
@sreichel I.e. it will break code where:
|
Hi @sreichel we had a group code review session today discussing this pull request, we decided that is will be unsafe to replace the interface on this stage. It's ok to mark the interface as deprecated for now. Then after a couple of releases with it being deprecated, it will be ok to replace it. |
@sivaschenko is it OK to set |
@sreichel I am not sure about internal tickets, you can do it in this pull request as I think anyway this pull request will have to be recreated by the time it will be OK to deliver it. |
@sreichel the pull request is put on hold. Please open another pull request with a link to this one where an interface is marked as deprecated |
It is necessary to deprecate the interface before removing it from inheritance hierarchy. This pull request will be processed a couple of releases after #19601 is merged |
@sivaschenko are you sure it can be processed earlier than |
@orlangur In my opinion it can be processed earlier, however, no, I am not sure. |
@sivaschenko keeping open pull request is not a proper way to track necessary changes. Please report internal ticket targeting |
Hi @sreichel, thank you for your contribution! |
Changed my mind. |
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.
Hi @sreichel , we are going to resume processing of this pull request.
Can you please update your branch, resolve conflicts and ensure all tests are passing
The pull request is approved to be merged to 2.4 at magento/architecture#174 including the removal of ArrayInterface |
I'll update for 2.4. |
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.
Hi @sreichel,
Branch 2.4-develop
was already created and this PR was re-targeted into it. Could you update your PR and resolve merge conflicts?
Hi @sreichel, |
@sreichel I am closing this PR now due to inactivity. |
Hi @sreichel, thank you for your contribution! |
Description (*)
Replaced all occurrences and marked ArrayInterface as deprecated.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)