Skip to content

#9151: [Github] Sitemap.xml: lastmod timestamp can contain invalid dates #11902

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

Merged
merged 2 commits into from
Nov 9, 2017

Conversation

serhii-balko
Copy link
Contributor

Description

Allow only valid lastmod values in Sitemap

Fixed Issues

  1. Sitemap.xml: lastmod timestamp can contain invalid dates #9151: Sitemap.xml: lastmod timestamp can contain invalid dates

Manual testing scenarios

  1. Create Simple Product with sku='simple1' ;
  2. Update field 'updated_at' by query:
SET sql_mode = '';
UPDATE catalog_product_entity SET updated_at = '0000-00-00 00:00:00' WHERE sku = 'simple1';
  1. Generate sitemap.
  2. The XML file should not contain
    <lastmod>-001-11-30T00:00:00+00:00</lastmod>
  3. Lastmod vlaue should be like this:
    <lastmod>0000-01-01T00:00:00+00:00</lastmod>

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)

@dmanners dmanners self-assigned this Nov 1, 2017
@dmanners dmanners added this to the November 2017 milestone Nov 1, 2017
@dmanners dmanners added Release Line: 2.2 2.2.x bug report Fixed in 2.3.x The issue has been fixed in 2.3 release line Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Nov 1, 2017
Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

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

I have a question about the result of this PR compared to the issue request.

*
* @var int
*/
protected $lastModMinTsVal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a new property can we make it private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -661,7 +673,11 @@ protected function _getMediaUrl($url)
*/
protected function _getFormattedLastmodDate($date)
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see in the discussion in #9151 the talked about solution would be to show The lastmod value in the xml should contain the created_at timestamp was this changed at some point to show 0000-01-01 00:00:00 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @dmanners
Magento should not be obliged to respond to incorrectly filled database fields through third-party code. But in the formation of sitemap we must take into account the case when the lastmod has zero value.
In the case of the #9151, the field "created_at" may also contain a zero value, and the issue remains.
So I decided that is the better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the update that has clarified it nicely.

@okorshenko okorshenko merged commit 4177336 into magento:2.2-develop Nov 9, 2017
okorshenko pushed a commit that referenced this pull request Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report bugfix Fixed in 2.3.x The issue has been fixed in 2.3 release line Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants