-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Removed logic related to columnCountLayoutDepend #19349
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 |
|
Hi @sreichel , thanks for your contribution. Unfortunately removing public/protected functions and properties are backward incompatible changes and cannot be accepted. For now it would be good to mark the functions you think should be removed as deprecated. I would also get an architecural approval for such changes by creating a pull request with proposition here: https://github.com/magento/architecture |
orlangur
left a comment
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.
@sreichel nice!
Please remove testLayoutDependColumnCount completely as it does not make a lot of sense now and we are good to go.
|
Hi @orlangur, thank you for the review. |
sivaschenko
left a comment
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 the changes introduced in this pull request are backward incompatible, if you'd like to remove _columnCountLayoutDepend - please mark it deprecated but do not eliminate any usages/functionality yet.
|
@sivaschenko your statement is incorrect. This is Magento 1 reminant and never did anything useful in Magento 2.0 GA or later. |
|
Hi @orlangur, thank you for the review. |
|
@sivaschenko @sidolov please clarify if anything left to be done here taking into consideration #19349 (comment). |
| * | ||
| * @var array | ||
| */ | ||
| protected $_columnCountLayoutDepend = []; |
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.
Please, mark property as deprecated due to backward compatibility instead of remove.
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.
Thanks Stas, somehow I overlooked it, while "do not eliminate any usages/functionality yet" looks completely not correct to me.
|
Hi @sreichel, thank you for your contribution! |
Description (*)
See comment at #19309
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)