Skip to content

#11936:required attribute set id filter on attribute group repository getList #12105

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

Merged
merged 2 commits into from
Dec 22, 2017

Conversation

tzyganu
Copy link
Contributor

@tzyganu tzyganu commented Nov 8, 2017

Description

This PR removes the need for an attribute_set_id filter in the method Magento\Eav\Model\Attribute\GroupRepository::getList

Fixed Issues (if relevant)

  1. required attribute set id filter on attribute group repository getList #11936: required attribute set id filter on attribute group repository getList

Manual testing scenarios

  1. There aren't any since this is more of a technical task than a user facing issue fix

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)

Copy link
Contributor

@mzeis mzeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked @maghamed for his approval because of discussions in the issue. Otherwise, changes are fine for me.

@@ -117,16 +117,6 @@ public function save(\Magento\Eav\Api\Data\AttributeGroupInterface $group)
*/
public function getList(\Magento\Framework\Api\SearchCriteriaInterface $searchCriteria)
{
$attributeSetId = $this->retrieveAttributeSetIdFromSearchCriteria($searchCriteria);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maghamed Since you requested the attribute set to be optional in #11936 (comment): are you fine with it being removed completely?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mzeis and @tzyganu
I am fine with removing this check,
The only thing we need to add here is to mark method
protected function retrieveAttributeSetIdFromSearchCriteria(
as @deprecated because we no need it any more, but because it's protected we can't just remove it

@magento-engcom-team magento-engcom-team added Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 21, 2017
Copy link
Contributor

@mzeis mzeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tzyganu
Copy link
Contributor Author

tzyganu commented Dec 20, 2017

@mzeis @maghamed I did the requested changes. Sorry for answering so late. I completely missed this.

@mzeis
Copy link
Contributor

mzeis commented Dec 20, 2017

@tzyganu Thanks for the change, don't worry!

@mzeis mzeis added this to the December 2017 milestone Dec 20, 2017
@magento-team magento-team merged commit 7be92a8 into magento:2.2-develop Dec 22, 2017
magento-team pushed a commit that referenced this pull request Dec 22, 2017
@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 22, 2017
@mzeis
Copy link
Contributor

mzeis commented Dec 23, 2017

@tzyganu Many thanks for your contribution. It has been merged. Happy holidays! 🎄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Eav Fixed in 2.2.x The issue has been fixed in 2.2 release line improvement Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants