Skip to content
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.

Sign up for GitHub

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

Fix customer group extension attributes for lists #53

Open
wants to merge 1 commit into
base: 2.4-develop
Choose a base branch
from
Open

Fix customer group extension attributes for lists #53

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/code/Magento/Customer/Model/Plugin/GetListCustomerGroupExcludedWebsite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ public function afterGetList(
$customerGroupId = (int)$customerGroup->getId();
if (array_key_exists($customerGroupId, $allExcludedWebsites)) {
$excludedWebsites = $allExcludedWebsites[$customerGroupId];
$customerGroupExtensionAttributes = $this->groupExtensionInterfaceFactory->create();
$customerGroupExtensionAttributes = $customerGroup->getExtensionAttributes();
if ($customerGroupExtensionAttributes === null) {
Copy link
Contributor

@Den4ik Den4ik Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

getExtensionAttributes comment block contain information that function could return null, but in fact it never be null, see https://github.com/mage-os/mageos-magento2/blob/2.4-develop/lib/internal/Magento/Framework/Api/AbstractExtensibleObject.php#L169-L171
I believe that object creation could be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Den4ik Good catch!
Having that in mind, I would replace this if block to following:

                    if (array_key_exists($customerGroupId, $allExcludedWebsites)) {
                        $excludedWebsites = $allExcludedWebsites[$customerGroupId];

                        // extension attributes are always there
                        /** @var GroupExtensionInterface $customerGroupExtensionAttributes */
                        $customerGroupExtensionAttributes = $customerGroup->getExtensionAttributes();
                        $customerGroupExtensionAttributes->setExcludeWebsiteIds($excludedWebsites);
                    }

Also, it would be great to fix the return type in \Magento\Customer\Api\Data\GroupInterface::getExtensionAttributes and \Magento\Customer\Model\Data\Group::getExtensionAttributes, but I'm not sure if we can do that as a part of mage-os.

Copy link
Contributor

@ihor-sviziev ihor-sviziev Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Additional idea - since the interface declares it can be null, there might be a case when the custom extension can override the value to null for some reason, and this code will start failing, so maybe having this fallback to object creation might be not that bad thing.
So now I prefer keeping the original fix

Copy link
Contributor

Choose a reason for hiding this comment

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

@ihor-sviziev I'm totally agree that interface method declaration should be fixed as weel. Good point for Magento Core PR, but it's backward incompatible changes that will produce long acceptance process by the Core Team.

Additional idea - since the interface declares it can be null, there might be a case when the custom extension can override the value to null for some reason, and this code will start failing, so maybe having this fallback to object creation might be not that bad thing.

I have seen many modules but don't faced with situation where it replace extension attributes to null. So I think we can stay with $customerGroupExtensionAttributes = $customerGroup->getExtensionAttributes();

$customerGroupExtensionAttributes = $this->groupExtensionInterfaceFactory->create();
}
$customerGroupExtensionAttributes->setExcludeWebsiteIds($excludedWebsites);
$customerGroup->setExtensionAttributes($customerGroupExtensionAttributes);
}
Expand Down
Loading