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

Correctly process global product attributes #5941 #8999

Merged
merged 4 commits into from Jul 19, 2017

Conversation

@therool
Copy link
Contributor

commented Mar 24, 2017

Description

Correctly process global product attributes when generating sitemap and added public method to process select statement before executing it so it can be modified with plugins. This is an fix for #5941

Fixed Issues (if relevant)

  1. #5941: If name attribute scope is changed to global, sitemap crashes

Manual testing scenarios

  1. Change store to single store view mode
  2. Make "name", "thumbnail", "image" product attributes global
  3. Go to "Marketing -> Site Map"
  4. Press "Add Site Map"
  5. Enter "sitemap.xml" as "Filename" and "/pub/" as "Path"
  6. Press "Save & Generate"

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-cicd2

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2017

CLA assistant check
All committers have signed the CLA.

@okorshenko okorshenko self-assigned this Jul 13, 2017

@okorshenko okorshenko added this to the July 2017 milestone Jul 13, 2017

@okorshenko okorshenko added the develop label Jul 13, 2017

@okorshenko

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

Hi @therool
Could you please fix broken tests so that we can proceed with Pull Request acceptance?
Thank you

@therool

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

Hello, as I understhand then this is this the test which failed:

Time: 2.8 minutes, Memory: 220.25MB
There was 1 failure:
1) Magento\Test\Php\LiveCodeTest::testCodeStyle
PHP Code Sniffer detected 1 violation(s): 
FILE: ...gento2/app/code/Magento/Sitemap/Model/ResourceModel/Catalog/Product.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 439 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------
Failed asserting that 1 matches expected 0.
/home/travis/build/magento/magento2/dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php:210
                                                 
FAILURES!                                        
Tests: 3, Assertions: 2, Failures: 1, Skipped: 1.

That line contains a function from other than my commit https://github.com/therool/magento2/blob/faa7cd1fb0a217fd128867ce6fac6ec7d477b831/app/code/Magento/Sitemap/Model/ResourceModel/Catalog/Product.php#L439 and that seems to be issue not related to my provided changes.

I found that the issue is really on the line 433 and is due the function docblock being incorrectly defined - it is:

    /*
     * Get product image URL from image filename and path
     *
     * @param string $image
     * @return string
     */

but needs to be:

   /**
     * Get product image URL from image filename and path
     *
     * @param string $image
     * @return string
     */

It is missing additional * after the /*, do I need to push this docblock fix?

@okorshenko

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

Hi @therool that would great if you can fix this issue so that we can make sure that your changes do not fail the builds. Thank you

@magento-team magento-team merged commit b1c1511 into magento:develop Jul 19, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
magento-team pushed a commit that referenced this pull request Oct 10, 2017
magento-team pushed a commit that referenced this pull request Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.