Skip to content
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

Remove Environment emulation for better performance on sitemap generation #21542

Merged
merged 2 commits into from
Mar 14, 2019
Merged

Remove Environment emulation for better performance on sitemap generation #21542

merged 2 commits into from
Mar 14, 2019

Conversation

Nazar65
Copy link
Member

@Nazar65 Nazar65 commented Mar 1, 2019

Description (*)

We dont need to use Environment emulation we just need to add in

protected function _loadProductImages($product, $storeId)
this $this->_storeManager->setCurrentStore($storeId);

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title
  2. ...

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

1.2 - Same steps for manual generating sitemap

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
Copy link
Contributor

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

@Nazar65 Nazar65 changed the title Remove Environment emulation to better performance on sitemap generation Remove Environment emulation for better performance on sitemap generation Mar 1, 2019
@orlangur orlangur self-assigned this Mar 1, 2019
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

Seems to be a reworked version of #16813 and https://github.com/magento/magento2/pull/19598/files. Could you please remove unneeded dependencies then?

@sidolov @sivaschenko would be really nice if you change the logic as you reviewed older PR.

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 1, 2019

@orlangur Thanks, for some reason i forgot for this.

@Nazar65 Nazar65 requested a review from sivaschenko March 1, 2019 15:17
Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Hi @Nazar65 thanks for the pull request. The functionality looks fine, however, I don't think we can remove the arguments from construction at this time for backward compatibility purpose. Can you please preserve constructor signatures

@orlangur
Copy link
Contributor

orlangur commented Mar 5, 2019

@sivaschenko not really. We cannot change released class signature (@Nazar65 please revert those) but we are free to change the 2.3.2 targeted PR.

@magento-engcom-team
Copy link
Contributor

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

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

@Nazar65 unneeded dependency introduced in https://github.com/magento/magento2/pull/19598/files must be removed.

Please squash into single commit after change.

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 5, 2019

@orlangur ok

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 5, 2019

@orlangur done ✔️

@sivaschenko
Copy link
Member

@orlangur I see what you mean, yes, those changes are not included in 2.3.1 👍

@magento-engcom-team
Copy link
Contributor

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

@fooman
Copy link
Contributor

fooman commented Mar 5, 2019

Judging by the manual testing steps this is currently using English as a language only. Emulation also sets different locales, reloads translations and switches the area. This all seems to be removed in this PR.

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 6, 2019

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @Nazar65. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @Nazar65, here is your new Magento instance.
Admin access: https://pr-21542.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 6, 2019

@fooman No, everything works as expected from manual steps, and this works perfect on 2.2.7 without emulation.

@soleksii
Copy link

✔️ QA Passed

@sidolov
Copy link
Contributor

sidolov commented Mar 11, 2019

Hi @Nazar65 , looks like these changes were introduced to fix the bug with sitemap generation in scope of #15689. Could you please verify that initial case not reproducing with provided changes?
Thank you!

@Nazar65
Copy link
Member Author

Nazar65 commented Mar 12, 2019

@sidolov Yes, initial case not reproducing with provided changes

@ghost ghost assigned sidolov Mar 12, 2019
@magento-engcom-team magento-engcom-team merged commit 348eeb6 into magento:2.3-develop Mar 14, 2019
@ghost
Copy link

ghost commented Mar 14, 2019

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.

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.

8 participants