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

Nazar65 · 2018-12-06T14:50:07Z

Description (*)

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

Fixed Issues (if relevant)

Images in XML sitemap are always linked to base store in multistore on Schedule #19596: Images in XML sitemap are always linked to base store in multistore on Schedule
...

Manual testing scenarios (*)

Website URL 1: https://my_website.com
Website URL 2: https://my_website.co.uk

Got to Stores->Configuration->Catalog->XML Sitemap->General Settings:
2.Set to Enable, Start Time, Frequency Daily etc...
TIP: I've set to a few minutes before checking!

Website 1 should have images url referencing Website 1 domain = https://my_website.com

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

magento-engcom-team · 2018-12-06T14:50:13Z

Hi @Nazar65. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

sivaschenko

@Nazar65 thanks for the contribution. Please see the code review notes.

sivaschenko · 2018-12-07T09:08:01Z

app/code/Magento/Sitemap/Model/Observer.php

@@ -67,13 +69,21 @@ class Observer
    protected $inlineTranslation;

    /**
+     * @var \Magento\Store\Model\App\Emulation $appEmulation
+     */
+    protected $appEmulation;


please change the access level to private

app/code/Magento/Sitemap/Model/Observer.php

sivaschenko · 2018-12-07T09:09:16Z

app/code/Magento/Sitemap/Model/Observer.php

     * @param \Magento\Store\Model\StoreManagerInterface $storeManager
     * @param \Magento\Framework\Mail\Template\TransportBuilder $transportBuilder
     * @param \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
     */
    public function __construct(
+        Emulation $appEmulation = null,


Please add move the new parameter to the last position in the parameters list

Nazar65 · 2018-12-07T12:14:16Z

@sivaschenko all done.

sivaschenko

@Nazar65 thanks for your updates, please see remaining review notes

app/code/Magento/Sitemap/Model/Observer.php

sivaschenko · 2018-12-07T13:37:35Z

app/code/Magento/Sitemap/Model/Observer.php

        \Magento\Store\Model\StoreManagerInterface $storeManager,
        \Magento\Framework\Mail\Template\TransportBuilder $transportBuilder,
-        \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
+        \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation,
+        Emulation $appEmulation


This new parameter should be optional

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.

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

sivaschenko · 2018-12-07T13:37:47Z

app/code/Magento/Sitemap/Model/Observer.php

@@ -112,9 +123,17 @@ public function scheduledGenerateSitemaps()
        foreach ($collection as $sitemap) {
            /* @var $sitemap \Magento\Sitemap\Model\Sitemap */
            try {
+                $this->appEmulation->startEnvironmentEmulation(
+                    $sitemap->getStoreId(),
+                    'frontend',


Please use the constant

This Class have many dependency, so if i will use constant the codeMess test will fail.

In this case it's better to decompose the class in favour to dropping constant usage

Co-Authored-By: Nazar65 <[email protected]>

Nazar65 · 2018-12-07T13:53:08Z

@sivaschenko please look at comments.

sivaschenko · 2018-12-07T13:57:51Z

Hi @Nazar65 , I think the best way to resolve the problems with static tests and keep the good code style and backward compatibility - is extracting all the business logic to a model from the observer. Actually, this observer is a very good candidate for refactoring. Could you extract it?

Nazar65 · 2018-12-07T14:03:44Z

@sivaschenko yes i agree with you. I'm will work on this :)

# Conflicts: # app/code/Magento/Sitemap/Model/Observer.php

Nazar65 · 2018-12-11T12:00:09Z

Hi @sivaschenko can you please look at newly commit ?

sivaschenko

Hi @Nazar65 , thanks for your refactoring! It looks great, however, I have some imporvement suggestions. Please see my code review notes.

sivaschenko · 2018-12-11T14:41:45Z

app/code/Magento/Sitemap/Model/Observer.php

@@ -24,21 +31,6 @@ class Observer
     */
    const XML_PATH_CRON_EXPR = 'crontab/default/jobs/generate_sitemaps/schedule/cron_expr';

-    /**


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

sivaschenko · 2018-12-11T14:44:48Z

app/code/Magento/Sitemap/Model/Observer.php

     */
-    protected $_transportBuilder;
+    protected $_storeManager;


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

sivaschenko · 2018-12-11T14:45:58Z

app/code/Magento/Sitemap/Model/Observer.php

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


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

sivaschenko · 2018-12-11T14:48:45Z

app/code/Magento/Sitemap/Model/SitemapSendEmail.php

+            self::XML_PATH_ERROR_RECIPIENT,
+            ScopeInterface::SCOPE_STORE
+        );
+        if ($errors && $recipient) {


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

sivaschenko · 2018-12-11T14:53:02Z

app/code/Magento/Sitemap/Model/SitemapSendEmail.php

+     *
+     * @var \Magento\Framework\App\Config\ScopeConfigInterface
+     */
+    private $_scopeConfig;


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

sivaschenko · 2018-12-11T14:53:48Z

app/code/Magento/Sitemap/Model/SitemapSendEmail.php

@@ -0,0 +1,109 @@
+<?php


Please use strict mode in for all new PHP files

sivaschenko · 2018-12-11T14:56:17Z

app/code/Magento/Sitemap/Model/Observer.php

-        \Magento\Framework\Translate\Inline\StateInterface $inlineTranslation
+        ScopeConfigInterface $scopeConfig,
+        CollectionFactory $collectionFactory,
+        StoreManagerInterface $storeManager,


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

sivaschenko · 2018-12-11T15:02:05Z

app/code/Magento/Sitemap/Model/SitemapSendEmail.php

+        if ($errors && $recipient) {
+            $this->inlineTranslation->suspend();
+
+            $this->_transportBuilder->setTemplateIdentifier(


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

sivaschenko · 2018-12-11T15:07:10Z

app/code/Magento/Sitemap/Model/SitemapSendEmail.php

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


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 · 2018-12-12T12:43:48Z

Hi @sivaschenko all done. :)

sivaschenko

Hi @Nazar65 thanks for the refactoring! I've added some code suggestions. You can simply add all my suggestions to batch and commit them.

app/code/Magento/Sitemap/Model/EmailNotification.php

Nazar65 · 2018-12-12T15:45:54Z

@sivaschenko all done, thank you for the review :)

sivaschenko · 2018-12-12T16:09:52Z

Thank you @Nazar65 finally there are currently 14 commits, would you like to squash them into a single one?

Nazar65 · 2018-12-12T20:11:50Z

@sivaschenko sorry for that, now i "squash" to one commit.

magento-engcom-team · 2018-12-12T22:39:23Z

Hi @sivaschenko, thank you for the review.
ENGCOM-3683 has been created to process this Pull Request

app/code/Magento/Sitemap/Model/Observer.php

Nazar65 · 2019-01-17T09:00:54Z

@sivaschenko @sidolov so do I change the variables again?

app/code/Magento/Sitemap/Model/Observer.php

magento-engcom-team · 2019-01-17T16:13:21Z

Hi @sidolov, thank you for the review.
ENGCOM-3683 has been created to process this Pull Request

Nazar65 · 2019-02-07T10:14:22Z

HI @sivaschenko @sidolov any updates ?

sivaschenko · 2019-02-07T18:06:10Z

Hi @Nazar65 the pull request is in queue for delivery to mainline

ghost · 2019-02-13T15:46:50Z

Hi @Nazar65, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

… multistore on Schedule #19598

fix issue-19596 Images in XML sitemap

d5b0ffd

magento-engcom-team added Component: Sitemap Release Line: 2.3 labels Dec 6, 2018

sivaschenko requested changes Dec 7, 2018

View reviewed changes

sivaschenko self-assigned this Dec 7, 2018

Fix failed static tests

16f11f7

sivaschenko requested changes Dec 7, 2018

View reviewed changes

Update app/code/Magento/Sitemap/Model/Observer.php

c8afe9f

Co-Authored-By: Nazar65 <[email protected]>

Nazar65 added 3 commits December 11, 2018 13:54

Refactoring Sitemap Observer

fda07a2

Merge remote-tracking branch 'origin/issue-19596' into issue-19596

5c7527e

# Conflicts: # app/code/Magento/Sitemap/Model/Observer.php

Refactoring Sitemap Observer

d2d04b6

sivaschenko requested changes Dec 11, 2018

View reviewed changes

Refactoring

b88fa48

Refactoring

85a2a56

sivaschenko requested changes Dec 12, 2018

View reviewed changes

sivaschenko approved these changes Dec 12, 2018

View reviewed changes

magento-engcom-team added this to the Release: 2.3.1 milestone Dec 12, 2018

ENGCOM-3683: Unit test fix.

a04cbf2

magento-engcom-team added the Progress: accept label Dec 21, 2018

sidolov requested changes Jan 4, 2019

View reviewed changes

app/code/Magento/Sitemap/Model/Observer.php Show resolved Hide resolved

sidolov added the Progress: needs update label Jan 4, 2019

sivaschenko removed the Progress: needs update label Jan 17, 2019

sidolov approved these changes Jan 17, 2019

View reviewed changes

app/code/Magento/Sitemap/Model/Observer.php Show resolved Hide resolved

magento-engcom-team assigned sidolov Jan 17, 2019

okorshenko removed this from the Release: 2.3.1 milestone Jan 28, 2019

ghost mentioned this pull request Feb 7, 2019

Magento 2 sitemap wrong paths in sitemap index when running on cron #19565

Closed

magento-engcom-team mentioned this pull request Feb 13, 2019

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

Closed

magento-engcom-team merged commit a04cbf2 into magento:2.3-develop Feb 13, 2019

magento-engcom-team pushed a commit that referenced this pull request Feb 13, 2019

ENGCOM-3683: Images in XML sitemap are always linked to base store in…

cb08046

… multistore on Schedule #19598

magento-engcom-team added the Award: bug fix label Feb 13, 2019

sidolov added this to the Release: 2.3.2 milestone Feb 13, 2019

Nazar65 mentioned this pull request Feb 14, 2019

[Backport]-issue-195196 Images in XML sitemap are always linked to base store in multistore on Schedule #21220

Closed

4 tasks

	$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();
	}

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

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

Conversation

Nazar65 commented Dec 6, 2018

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Dec 6, 2018

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nazar65 commented Dec 7, 2018

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nazar65 commented Dec 7, 2018

sivaschenko commented Dec 7, 2018

Nazar65 commented Dec 7, 2018

Nazar65 commented Dec 11, 2018

sivaschenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nazar65 commented Dec 12, 2018

sivaschenko left a comment

Choose a reason for hiding this comment

Nazar65 commented Dec 12, 2018

sivaschenko commented Dec 12, 2018

Nazar65 commented Dec 12, 2018

magento-engcom-team commented Dec 12, 2018

Nazar65 commented Jan 17, 2019

magento-engcom-team commented Jan 17, 2019

Nazar65 commented Feb 7, 2019

sivaschenko commented Feb 7, 2019

ghost commented Feb 13, 2019