Skip to content

Conversation

@yogeshsuhagiya
Copy link
Member

@yogeshsuhagiya yogeshsuhagiya commented Mar 10, 2018

Hello,

Description

Removed unnecessary protected member variables of Image class.
The variable $imageHelper, $product and $attributes are not in use.
Above three variable doesn't contain any values. Also verified it on category, product and cart page.

Fixed Issues (if relevant)

N/A

Manual testing scenarios

N/A

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

Removed unnecessary protected member variables of Image class.
The variable `$imageHelper`, `$product` and `$attributes` are not in use.
Above three variable doesn't contain any values. Also verified it on category, product and cart page.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 10, 2018

CLA assistant check
All committers have signed the CLA.

@mzeis mzeis self-assigned this Mar 10, 2018
@mzeis
Copy link
Contributor

mzeis commented Mar 10, 2018

Hi @yogeshks,

thanks for your contribution! Unfortunately, removing protected properties is not allowed by the backwards incompatible development guide. I see that you are saying the properties aren't used - I can check on that. We have to be careful though to not break any behaviour in third party code if this should go into 2.2-develop.

What you could do according to the guide is to tag the properties as @deprecated in 2.2-develop and create another PR for 2.3-develop, removing them.

Restored protected properties of Image class and marked them as `@deprecated`.
@yogeshsuhagiya
Copy link
Member Author

Hi @mzeis

Thank you so much for your guidance, I've restored protected properties and marked them as @deprecated as per your suggestion.

@orlangur
Copy link
Contributor

@mzeis what's the point in such comments? They should be added or properties should be removed all at once with some automated tooling.

@mzeis
Copy link
Contributor

mzeis commented Mar 11, 2018

Hi @yogeshks, I updated your PR:

  • Added an explanation why the property is deprecated
  • Removed "deprecated since" information (this will be added automatically for a new release)
  • Removed added whitespace

You don't need to create a PR for 2.3-develop as the properties will be removed automatically.

@magento-engcom-team magento-engcom-team added this to the March 2018 milestone Mar 11, 2018
@magento-engcom-team
Copy link
Contributor

Hi @mzeis, thank you for the review.
ENGCOM-847 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@yogeshks thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@mzeis
Copy link
Contributor

mzeis commented Mar 18, 2018

Hi @yogeshks, thanks for your PR! It has been merged into 2.2-develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants