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

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

Closed
hostep opened this issue Apr 6, 2017 · 5 comments
Closed

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

hostep opened this issue Apr 6, 2017 · 5 comments
Assignees
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed 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

Comments

@hostep
Copy link
Contributor

hostep commented Apr 6, 2017

Preconditions

  1. Magento CE 2.1.5

Steps to reproduce

  1. Have a category in the database where the created_at timestamp is a valid date, and the updated_at timestamp is 0000-00-00 00:00:00
    I don't think you can manage this by manually creating a category in the backend of Magento, but we managed to have some categories like this by importing them using a custom PHP script.
  2. Generate a sitemap.xml file
  3. Look at the generated sitemap.xml file for that category.

Expected result

  1. The lastmod value in the xml should contain the created_at timestamp

Actual result

  1. The lastmod value in the xml looks like this:
<lastmod>-001-11-30T00:00:00+00:00</lastmod>

The negative value is most likely caused by our timezone setting: Europe/Brussels.

Possible solution

diff --git a/app/code/Magento/Sitemap/Model/Sitemap.php b/app/code/Magento/Sitemap/Model/Sitemap.php
index 3b6f646f9a5..434393ae63a 100644
--- a/app/code/Magento/Sitemap/Model/Sitemap.php
+++ b/app/code/Magento/Sitemap/Model/Sitemap.php
@@ -341,9 +341,10 @@ class Sitemap extends \Magento\Framework\Model\AbstractModel
             $changefreq = $sitemapItem->getChangefreq();
             $priority = $sitemapItem->getPriority();
             foreach ($sitemapItem->getCollection() as $item) {
+                $lastmod = $item->getUpdatedAt() === '0000-00-00 00:00:00' ? $item->getCreatedAt() : $item->getUpdatedAt();
                 $xml = $this->_getSitemapRow(
                     $item->getUrl(),
-                    $item->getUpdatedAt(),
+                    $lastmod,
                     $changefreq,
                     $priority,
                     $item->getImages()

Discussion

I'm not sure if this is an actual bug in Magento, due to the fact that we inserted some data in the database in a possibly incorrect way.

Also: what should happen when the created_at timestamp contains '0000-00-00 00:00:00'? In that case the lastmod field shouldn't appear I assume, since it is optional: https://www.sitemaps.org/protocol.html#xmlTagDefinitions

If somebody could let me know what the best solution would be in this case, I can create a pull request for this.
If it is decided that this isn't an actual bug, then we should fix our custom import code I guess.

Please let me know, thanks! :)

@korostii
Copy link
Contributor

korostii commented Apr 6, 2017

IMHO setting updated_at to a meaningful value during the import seems most logical, regardless of the outcome of this issue.

Back to the issue in question, a negative date value is something new 🤣

@magento-engcom-team
Copy link
Contributor

@hostep, thank you for your report.
The issue is already fixed in develop branch
But we will consider to backport the fix to patch releases

@hostep
Copy link
Contributor Author

hostep commented Oct 11, 2017

@magento-engcom-team: can you point me to the commit which fixes this, since I don't see it?
The last change to the last <lastmod> in the Magento\Sitemap\Model\Sitemap class was done 4 years ago ...

I think you are talking about issue 10598 which is not the same as this one :)

@magento-engcom-team magento-engcom-team added 2.2.x Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed 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 Oct 12, 2017
@serhii-balko serhii-balko self-assigned this Oct 31, 2017
serhii-balko added a commit to serhii-balko/magento2 that referenced this issue Oct 31, 2017
serhii-balko added a commit to serhii-balko/magento2 that referenced this issue Nov 2, 2017
okorshenko pushed a commit that referenced this issue Nov 9, 2017
…tain invalid dates #11902

 - Merge Pull Request #11902 from serhii-balko/magento2:github-9151
 - Merged commits:
   1. 0212c14
   2. 4177336
okorshenko pushed a commit that referenced this issue Nov 9, 2017
@okorshenko
Copy link
Contributor

The issue has been fixed in 2.2-develop branch. Will be available with 2.2.2 release

@okorshenko okorshenko added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Nov 9, 2017
@okorshenko
Copy link
Contributor

Hi @hostep. Thank you for your report.
The issue has been fixed in magento-engcom/magento2ce#1353 by @magento-engcom-team in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed 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

No branches or pull requests

6 participants