Skip to content

Update AttributeRepository.php #9387

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

Closed
wants to merge 2 commits into from
Closed

Update AttributeRepository.php #9387

wants to merge 2 commits into from

Conversation

redelschaap
Copy link
Contributor

@redelschaap redelschaap commented Apr 25, 2017

Fixes a bug in case $attribute->getOption() returns NULL when it does not have any options. When NULL is given to AttributeRepository::getOptionArray, PHP complains about a TypeError.

Fixes a bug in case `$attribute->getOption()` returns NULL when it does not have any options. When NULL is given to `AttributeRepository::getOptionArray`, PHP complains about a TypeError.
@ishakhsuvarov ishakhsuvarov self-assigned this Apr 27, 2017
@ishakhsuvarov ishakhsuvarov added this to the April 2017 milestone Apr 27, 2017
Copy link
Contributor

@ishakhsuvarov ishakhsuvarov left a comment

Choose a reason for hiding this comment

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

Service contract for \Magento\Customer\Api\Data\AttributeMetadataInterface::getOptions (which is the $attribute in this case) states that the only possible return type is OptionInterface[]. It is better to check why is it null in the specific case and fix it.

@@ -95,12 +95,13 @@ protected function getListForEntity(array $metadata, $entityTypeCode, MetadataMa
if ($entityTypeCode == AddressMetadataInterface::ENTITY_TYPE_ADDRESS) {
$attributeCode = self::BILLING_ADDRESS_PREFIX . $attribute->getAttributeCode();
}
$attributeOptions = $attribute->getOption();
Copy link
Contributor

Choose a reason for hiding this comment

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

Original code used $attribute->getOptions() did you mean it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, don't know how it got there

$attributes[$attributeCode] = [
AttributeMetadataInterface::ATTRIBUTE_CODE => $attributeCode,
AttributeMetadataInterface::FRONTEND_INPUT => $attribute->getFrontendInput(),
AttributeMetadataInterface::FRONTEND_LABEL => $attribute->getFrontendLabel(),
AttributeMetadataInterface::BACKEND_TYPE => $attribute->getBackendType(),
AttributeMetadataInterface::OPTIONS => $this->getOptionArray($attribute->getOptions()),
AttributeMetadataInterface::OPTIONS => is_array($attributeOptions) ? $this->getOptionArray($attributeOptions) : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is too long. Magento code style standard requires lines to be no longer then 120 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@redelschaap
Copy link
Contributor Author

redelschaap commented Apr 28, 2017

@ishakhsuvarov Updated the code with your change requests.

@ishakhsuvarov
Copy link
Contributor

@redelschaap Please check the general comment as well:

Service contract for \Magento\Customer\Api\Data\AttributeMetadataInterface::getOptions (which is the $attribute in this case) states that the only possible return type is OptionInterface[]. It is better to check why is it null in the specific case and fix it.

I am not sure this is the most optimal fix at the moment

@redelschaap
Copy link
Contributor Author

I think this fix is optimal, because we have a situation where that method does not return an array of OptionInterfaces, but NULL instead. I don't remember what exactly caused it, but it the option property of that object was never filled and thus NULL.

@ishakhsuvarov
Copy link
Contributor

@redelschaap Service contract claims that no NULL value should be returned. Could you please check the case you have encountered? We may need to fix that as it may affect much more client code.

@redelschaap
Copy link
Contributor Author

@ishakhsuvarov That's the theory, practice shows otherwise ;). I can't reproduce the bug right now as we're working towards the launch of the website.

Where do you suggest this should be fixed?

@ishakhsuvarov
Copy link
Contributor

@redelschaap I would suggest finding exact case, where it reproduces, and fixing the reason why null is returned.

@ishakhsuvarov ishakhsuvarov modified the milestones: May 2017, April 2017 May 8, 2017
@redelschaap
Copy link
Contributor Author

Well in my case an extension was responsible, that I do know. You can't say "it's the extension's fault, they should fix it", when Magento's core makes it possible to leave the options property NULL.

@okorshenko okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017
@ishakhsuvarov
Copy link
Contributor

@redelschaap Closing this PR for now. Please reopen if this issue occurs again with the reproducible steps. We will definitely look for ways to solve the problem.
Thank you for collaboration.

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.

3 participants