Skip to content

Conversation

pbaylies
Copy link
Contributor

@pbaylies pbaylies commented Aug 9, 2017

Check to make sure that $this->getChildProduct() isn't null here before trying to call any class methods on it.

Check to make sure that $this->getChildProduct() isn't null here before trying to call any class methods on it.
@orlangur orlangur self-assigned this Aug 9, 2017
@@ -66,7 +66,7 @@ public function getProductForThumbnail()
if ($this->_scopeConfig->getValue(
self::CONFIG_THUMBNAIL_SOURCE,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
) == ThumbnailSource::OPTION_USE_PARENT_IMAGE ||
) == ThumbnailSource::OPTION_USE_PARENT_IMAGE || (empty($this->getChildProduct()) ||
!($this->getChildProduct()->getThumbnail() && $this->getChildProduct()->getThumbnail() != 'no_selection')
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest rewriting it in more readable form

!($this->getChildProduct() && $this->getChildProduct()->getThumbnail() && $this->getChildProduct()->getThumbnail() != 'no_selection')

When $this->getChildProduct() can be null by the way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orlangur We ran into this in production this morning, had a 500 error on /checkout/cart/ for a user and traced it back to this code. It was being called from the view/frontend/templates/cart/item/default.phtml template in the checkout module.

@orlangur orlangur added this to the August 2017 milestone Aug 9, 2017
@orlangur
Copy link
Contributor

orlangur commented Aug 9, 2017

Thanks for the quick change 👍 Please provide answer to the

When $this->getChildProduct() can be null by the way?

question to assure such change is really necessary.

@pbaylies
Copy link
Contributor Author

pbaylies commented Aug 9, 2017

@orlangur We ran into this in production this morning, had a 500 error on /checkout/cart/ for a user and traced it back to this code. It was being called from the view/frontend/templates/cart/item/default.phtml template in the checkout module.

@orlangur
Copy link
Contributor

orlangur commented Aug 9, 2017

@pbaylies do you have a stack trace maybe? As from \Magento\Quote\Model\Quote\Item\AbstractItem::getProduct there should have been fatal error on line 153 in such case.

Currently according to PHPDocs \Magento\Checkout\Block\Cart\Item\Renderer::getProduct and \Magento\ConfigurableProduct\Block\Cart\Item\Renderer\Configurable::getChildProduct can return product model only and I would like to understand whether it's problem with implementation or documentation.

@pbaylies
Copy link
Contributor Author

pbaylies commented Aug 9, 2017

@orlangur We have a partial stack trace from the logs:

Call to a member function getThumbnail() on null in /var/app/current/htdocs/vendor/magento/module-configurable-product/Block/Cart/Item/Renderer/Configurable.php:68

Stack trace:
#0 /var/app/current/htdocs/var/view_preprocessed/html/vendor/magento/module-checkout/view/frontend/templates/cart/item/default.phtml(1): Magento\ConfigurableProduct\Block\Cart\Item\Renderer\Configurable->getProductForThumbnail()
#1 /var/app/current/htdocs/vendor/magento/framework/View/TemplateEngine/Php.php(59): include('/var/app/curren...')
#2 /var/app/current/htdocs/vendor/magento/framework/View/Element/Template.php(255): Magento\Framework\View\TemplateEngine\Php->render(Object(Magento\ConfigurableProduct\Block\Cart\Item\Renderer\Configurable), '/var/app/curren...', Array)
#3 /var/app/current/htdocs/vendor/magento/framework/View/Element/Template.php(279): Magento\Framework\View\Element\Template->fetchView('/var/app/curren...')

@orlangur
Copy link
Contributor

orlangur commented Aug 9, 2017

Do you see an approximate way it can happen in vanilla Magento 2 CE? No customization could cause such behavior? Maybe you have access to database to observe quote item and execution flow in \Magento\ConfigurableProduct\Block\Cart\Item\Renderer\Configurable::getChildProduct and deeper.

@pbaylies
Copy link
Contributor Author

pbaylies commented Aug 9, 2017

@orlangur We are running EE with a lot of modules and customizations, so we can't really rule that out as a possibility. That said, there is a null value getting returned here, and it is causing a 500 error on the cart page, so, seems that is some cause for concern in this code.

@orlangur
Copy link
Contributor

Long story short, with the help of @maghamed I noticed that $option->getProduct() call does not follow OptionInterface thus it can potentially return null in some circumstances.

A better fix would be to find out when $option->getProduct() becoming null and fix this making proposed check redundant. If you won't manage to achieve this, let's just polish up current code:

  1. Move || to next line and format code accordingly so that there are no trailing spaces
  2. Change PHPDoc of \Magento\ConfigurableProduct\Block\Cart\Item\Renderer\Configurable::getChildProduct marking it can return product model or null

@pbaylies
Copy link
Contributor Author

@orlangur Thanks for tracking that down!

@orlangur
Copy link
Contributor

orlangur commented Aug 10, 2017

Hi @pbaylies,

A better fix would be to find out when $option->getProduct() becoming null and fix this making proposed check redundant

No luck with such approach or didn't try it? Please share your findings if any, if you're not going to try due to lack of time/motivation this is absolutely ok too.

@pbaylies
Copy link
Contributor Author

@orlangur With this fix, at least I know it catches all the cases where getChildProduct() might be null in this function. Figuring out when getProduct() might be null as well would be a bigger job for another ticket.

@orlangur
Copy link
Contributor

Aha, ok, let's leave as is (although we are fixing symptom and not a root of the problem here).

@pbaylies
Copy link
Contributor Author

@orlangur Thank you!

@magento-team magento-team merged commit eea585a into magento:develop Aug 10, 2017
magento-team pushed a commit that referenced this pull request Aug 10, 2017
@kilis
Copy link

kilis commented Jan 2, 2018

Hey!
Had the same issue and this was the fix for us also.
FYI this needs also needs to be fixed in here also Magento\ConfigurableProduct\CustomerData\ConfigurableItem:getProductForThumbnail() The same way.

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.

4 participants