Get sitemap product images from image cache, if available #9082
Conversation
|
Please use the E.g. // like this
ObjectManager::getInstance()->get(\Magento\Catalog\Model\Product::class);
// NOT like this
ObjectManager::getInstance()->get('Magento\Catalog\Model\Product'); |
|
Hi @adragus-inviqa Got any suggestions for PHPCS reporting warnings for too long line length, like 9c44917#diff-2cc96b8b46600260b67449219e565125R76 should those lines be ignored with "@codingStandardsIgnoreStart" comments or is there another solution I missed? |
|
They all seem to have the For me, |
|
How can I get some more info about why the previous build failed? https://travis-ci.org/magento/magento2/builds/217516099 The message doesn't help much: |
|
@sambolek I would suggest running unit tests on local env to see what is the problem in this case. |
| * @param string $image | ||
| * @return mixed | ||
| */ | ||
| protected function getProductImageUrl($image) |
There was a problem hiding this comment.
I would suggest making this private, as Magento discourages inheritance-based APIs.
| @@ -396,9 +396,28 @@ protected function _getAllProductImages($product, $storeId) | |||
| * Get media config | |||
| * | |||
| * @return \Magento\Catalog\Model\Product\Media\Config | |||
| * @deprecated No longer used, as we're getting full image URL from getProductImageUrl method | |||
There was a problem hiding this comment.
Please add @see annotation with the reference to the updated implementation.
| @@ -396,9 +396,28 @@ protected function _getAllProductImages($product, $storeId) | |||
| * Get media config | |||
| * | |||
| * @return \Magento\Catalog\Model\Product\Media\Config | |||
| * @deprecated No longer used, as we're getting full image URL from getProductImageUrl method | |||
| */ | |||
| protected function _getMediaConfig() | |||
| { | |||
| return $this->_mediaConfig; | |||
There was a problem hiding this comment.
Looks like you have eliminated all of the usages for this dependency.
Now you could also deprecate the property and remove all type hints to it.
| @@ -607,11 +607,12 @@ protected function _getUrl($url, $type = \Magento\Framework\UrlInterface::URL_TY | |||
| * Get media url | |||
| * | |||
| * @param string $url | |||
| * @return string | |||
| * @return string $url | |||
| * @deprecated No longer used, as we're generating product image URLs inside collection instead | |||
There was a problem hiding this comment.
Please add a @see annotation with the reference to the new approach
| */ | ||
| protected function getProductImageUrl($image) | ||
| { | ||
| $productObject = ObjectManager::getInstance()->get(\Magento\Catalog\Model\Product::class); |
There was a problem hiding this comment.
Consider adding these as an optional constructor injections
|
Fixed issues mentioned in review. Waiting for CI build to finish, but tested ok locally. Please let me know if there's anything else to fix (except potentially failed tests). |
…ble #9082 - Coding Style updated
…ble #9082 - Coding Style updated
Product images in sitemap are currently always pulled directly from gallery attributes by querying the database, whilst on frontend, we're using cached versions of the (that can potentially be stored somewhere else).
This can present an issue if google doesn't want to index images in the sitemap, as their URL is different than the one on product pages.
Description
In sitemap, product image URLs are now being pulled by using product helper, that can serve cached image, if available. This is in line to what we see on the frontend, on product page.
Fixed Issues (if relevant)
Manual testing scenarios
Contribution checklist