Skip to content

Images in XML sitemap are always linked to base store in multistore on Schedule #19598

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

Merged
magento-engcom-team merged 9 commits into from
Feb 13, 2019
Merged

Images in XML sitemap are always linked to base store in multistore on Schedule #19598

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d5b0ffd
fix issue-19596 Images in XML sitemap
Nazar65 Dec 6, 2018
16f11f7
Fix failed static tests
Nazar65 Dec 7, 2018
c8afe9f
Update app/code/Magento/Sitemap/Model/Observer.php
sivaschenko Dec 7, 2018
fda07a2
Refactoring Sitemap Observer
Nazar65 Dec 11, 2018
5c7527e
Merge remote-tracking branch 'origin/issue-19596' into issue-19596
Nazar65 Dec 11, 2018
d2d04b6
Refactoring Sitemap Observer
Nazar65 Dec 11, 2018
b88fa48
Refactoring
Nazar65 Dec 12, 2018
85a2a56
Refactoring
Nazar65 Dec 12, 2018
a04cbf2
ENGCOM-3683: Unit test fix.
p-bystritsky Dec 21, 2018
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
107 changes: 37 additions & 70 deletions app/code/Magento/Sitemap/Model/Observer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@
*/
namespace Magento\Sitemap\Model;

use Magento\Store\Model\App\Emulation;
use Magento\Sitemap\Model\SitemapSendEmail as SitemapEmail;
use Magento\Store\Model\StoreManagerInterface;
use Magento\Framework\App\Config\ScopeConfigInterface;
use Magento\Sitemap\Model\ResourceModel\Sitemap\CollectionFactory;
use Magento\Store\Model\ScopeInterface;

/**
Comment thread
Nazar65 marked this conversation as resolved.
* Sitemap module observer
*
Expand All @@ -24,21 +31,6 @@ class Observer
*/
const XML_PATH_CRON_EXPR = 'crontab/default/jobs/generate_sitemaps/schedule/cron_expr';

/**

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please keep all existing constants in the Observer as they could be already used by third-party extensions.

@Nazar65 Nazar65 Dec 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok.

* Error email template configuration
*/
const XML_PATH_ERROR_TEMPLATE = 'sitemap/generate/error_email_template';

/**
* Error email identity configuration
*/
const XML_PATH_ERROR_IDENTITY = 'sitemap/generate/error_email_identity';

/**
* 'Send error emails to' configuration
*/
const XML_PATH_ERROR_RECIPIENT = 'sitemap/generate/error_email';

/**
* Core store config
*
Expand All @@ -52,39 +44,40 @@ class Observer
protected $_collectionFactory;

/**
* @var \Magento\Framework\Mail\Template\TransportBuilder
* @var \Magento\Store\Model\StoreManagerInterface
*/
protected $_transportBuilder;
protected $_storeManager;

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd change all properties to private and remove the "_" prefix (for consistency and to be compatible with PSR)

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok


/**
* @var \Magento\Store\Model\StoreManagerInterface
* @var Emulation
*/
protected $_storeManager;
private $appEmulation;

/**
* @var \Magento\Framework\Translate\Inline\StateInterface
* @var $sitemapEmail
*/
protected $inlineTranslation;
private $sitemapEmail;

/**
* @param \Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig
* @param \Magento\Sitemap\Model\ResourceModel\Sitemap\CollectionFactory $collectionFactory
* @param \Magento\Store\Model\StoreManagerInterface $storeManager
* @param \Magento\Framework\Mail\Template\TransportBuilder $transportBuilder
* @param \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
* Observer constructor.
* @param ScopeConfigInterface $scopeConfig
* @param CollectionFactory $collectionFactory
* @param StoreManagerInterface $storeManager
* @param SitemapSendEmail $sitemapEmail
* @param Emulation $appEmulation
*/
public function __construct(
\Magento\Framework\App\Config\ScopeConfigInterface $scopeConfig,
\Magento\Sitemap\Model\ResourceModel\Sitemap\CollectionFactory $collectionFactory,
\Magento\Store\Model\StoreManagerInterface $storeManager,
\Magento\Framework\Mail\Template\TransportBuilder $transportBuilder,
\Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
ScopeConfigInterface $scopeConfig,
CollectionFactory $collectionFactory,
StoreManagerInterface $storeManager,

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Neither of two classes appears to be using store manager, so I think this dependency can be removed from both of them

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

SitemapEmail $sitemapEmail,
Emulation $appEmulation

@sivaschenko sivaschenko Dec 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This new parameter should be optional

@Nazar65 Nazar65 Dec 7, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same here if i add optional, as this have been in older commit the codeMess test will fail.
Backward Compatibility Policy is not applied to Plugins, Observers and Setup Scripts.

@Nazar65 Nazar65 Dec 7, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The class Observer has a coupling between objects value of 13.

) {
$this->_scopeConfig = $scopeConfig;
$this->_collectionFactory = $collectionFactory;
$this->_storeManager = $storeManager;
$this->_transportBuilder = $transportBuilder;
$this->inlineTranslation = $inlineTranslation;
$this->appEmulation = $appEmulation;
$this->sitemapEmail = $sitemapEmail;
}

/**
Expand All @@ -101,7 +94,7 @@ public function scheduledGenerateSitemaps()
// check if scheduled generation enabled
if (!$this->_scopeConfig->isSetFlag(
self::XML_PATH_GENERATION_ENABLED,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
ScopeInterface::SCOPE_STORE
)
) {
return;
Expand All @@ -112,46 +105,20 @@ public function scheduledGenerateSitemaps()
foreach ($collection as $sitemap) {
/* @var $sitemap \Magento\Sitemap\Model\Sitemap */
try {
$this->appEmulation->startEnvironmentEmulation(
$sitemap->getStoreId(),
\Magento\Framework\App\Area::AREA_FRONTEND,
true
);

$sitemap->generateXml();
} catch (\Exception $e) {
$errors[] = $e->getMessage();
}
}

if ($errors && $this->_scopeConfig->getValue(
self::XML_PATH_ERROR_RECIPIENT,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
)
) {
$this->inlineTranslation->suspend();

$this->_transportBuilder->setTemplateIdentifier(
$this->_scopeConfig->getValue(
self::XML_PATH_ERROR_TEMPLATE,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
)
)->setTemplateOptions(
[
'area' => \Magento\Backend\App\Area\FrontNameResolver::AREA_CODE,
'store' => \Magento\Store\Model\Store::DEFAULT_STORE_ID,
]
)->setTemplateVars(
['warnings' => join("\n", $errors)]
)->setFrom(
$this->_scopeConfig->getValue(
self::XML_PATH_ERROR_IDENTITY,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
)
)->addTo(
$this->_scopeConfig->getValue(
self::XML_PATH_ERROR_RECIPIENT,
\Magento\Store\Model\ScopeInterface::SCOPE_STORE
)
);
$transport = $this->_transportBuilder->getTransport();
$transport->sendMessage();

$this->inlineTranslation->resume();
$this->sitemapEmail->sendErrorEmail($errors);

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please call this method after the foreach (as all the errors have to be accumulated in $errors first)

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

} finally {
$this->appEmulation->stopEnvironmentEmulation();
}
}
}
}
109 changes: 109 additions & 0 deletions app/code/Magento/Sitemap/Model/SitemapSendEmail.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
<?php

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use strict mode in for all new PHP files

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Sitemap\Model;

use Magento\Framework\App\Config\ScopeConfigInterface;
use Magento\Framework\Translate\Inline\StateInterface;
use Magento\Framework\Mail\Template\TransportBuilder;
use Magento\Store\Model\ScopeInterface;
use Magento\Backend\App\Area\FrontNameResolver;
use Magento\Store\Model\Store;

/**
* Sends emails for the scheduled generation of the sitemap file
*/
class SitemapSendEmail

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd rename the class to the something like EmailNotification or Mail and the method to send or sendErrors to avoid duplicate wording in full method reference. Currently it's Magento\Sitemap\Model\SitemapSendEmail::sendErrorEmail

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

{
/**
* Error email template configuration
*/
const XML_PATH_ERROR_TEMPLATE = 'sitemap/generate/error_email_template';

/**
* Error email identity configuration
*/
const XML_PATH_ERROR_IDENTITY = 'sitemap/generate/error_email_identity';

/**
* 'Send error emails to' configuration
*/
const XML_PATH_ERROR_RECIPIENT = 'sitemap/generate/error_email';

/**
* @var \Magento\Framework\Translate\Inline\StateInterface
*/
private $inlineTranslation;

/**
* Core store config
*
* @var \Magento\Framework\App\Config\ScopeConfigInterface
*/
private $_scopeConfig;

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Property should not be prefixed with a single underscore, see https://www.php-fig.org/psr/psr-2/#42-properties

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok


/**
* @var \Magento\Framework\Mail\Template\TransportBuilder
*/
private $_transportBuilder;

/**
* SitemapSendEmail constructor.
* @param StateInterface $inlineTranslation
* @param TransportBuilder $transportBuilder
* @param ScopeConfigInterface $scopeConfig
*/
public function __construct(
StateInterface $inlineTranslation,
TransportBuilder $transportBuilder,
ScopeConfigInterface $scopeConfig
) {
$this->inlineTranslation = $inlineTranslation;
$this->_scopeConfig = $scopeConfig;
$this->_transportBuilder = $transportBuilder;
}

/**
* Send's error email if sitemap generated with errors.
*
* @param array $errors
* @throws \Magento\Framework\Exception\MailException
*/
public function sendErrorEmail($errors)
{
$recipient = $this->_scopeConfig->getValue(
self::XML_PATH_ERROR_RECIPIENT,
ScopeInterface::SCOPE_STORE
);
if ($errors && $recipient) {

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd move this condition to the observer or to a separate method, to keep this method solid

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

$this->inlineTranslation->suspend();

$this->_transportBuilder->setTemplateIdentifier(

@sivaschenko sivaschenko Dec 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it may be beneficial to wrap the email send operation in a try-catch block to gracefully handle situations when a mail server is not responsive.

There is an example here

magento2/app/code/Magento/Contact/Model/Mail.php

Lines 68 to 87 in f77f96b

$this->inlineTranslation->suspend();
try {
$transport = $this->transportBuilder
->setTemplateIdentifier($this->contactsConfig->emailTemplate())
->setTemplateOptions(
[
'area' => Area::AREA_FRONTEND,
'store' => $this->storeManager->getStore()->getId()
]
)
->setTemplateVars($variables)
->setFrom($this->contactsConfig->emailSender())
->addTo($this->contactsConfig->emailRecipient())
->setReplyTo($replyTo, $replyToName)
->getTransport();
$transport->sendMessage();
} finally {
$this->inlineTranslation->resume();
}

However, I would improve the approach by catching an exception and writing it to an exception log

@Nazar65 Nazar65 Dec 12, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

$this->_scopeConfig->getValue(
self::XML_PATH_ERROR_TEMPLATE,
ScopeInterface::SCOPE_STORE
)
)->setTemplateOptions(
[
'area' => FrontNameResolver::AREA_CODE,
'store' => Store::DEFAULT_STORE_ID,
]
)->setTemplateVars(
['warnings' => join("\n", $errors)]
)->setFrom(
$this->_scopeConfig->getValue(
self::XML_PATH_ERROR_IDENTITY,
ScopeInterface::SCOPE_STORE
)
)->addTo($recipient);

$transport = $this->_transportBuilder->getTransport();
$transport->sendMessage();

$this->inlineTranslation->resume();
}
}
}