Why are there calls to drupal_write_record() in xmlsitemap_output()? xmlsitemap_output() should only *output* the current sitemap. When we want to change the sitemap contents, this should be (re-)generated in hook_cron(), since this operation is extremely expensive (try viewing sitemap.xml using the test data I uploaded with 20,000 nodes, for example). It seems like the current behaviour could be very damaging from an SEO standpoint if search engines ping a sitemap URL and don't get a page returned to them within seconds.

I think this would also allow us to get rid of the confusing situation where you need to wrap this callback in a check for anonymous users.

Comments

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

The main problem is that the modules use their own database tables that are different from the table used to contain the site map links; this means the modules must implement a hook that allows them to create the links in the site map table, which need to be called at some point during the execution.
What I thought to do (in the 2.x branch) is to remove the module tables to use a central database table.

I think that the site map must not only filled during cron tasks execution, but also during the execution of other hooks, like hook_nodeapi(); using just cron tasks would make the process slowly, when there are hooks that operate on single objects at time (and for each page request).

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Postponed

The change suggested can only be implemented when the database tables used will be changed. I am setting this report as postponed for the next branch.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 6.x-1.x-dev » 6.x-2.x-dev

I am changing the referring version.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Postponed » Fixed

This is now and has been how the module operates in 6.x-2.x-dev. Marking as fixed.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 6.x-2.x-dev » 6.x-1.x-dev
Status: Fixed » Active

I am reopening the report because I would like to investigate if this is possible to implement in the 1.x branch.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

This will be implemented when #463564: Remove the central database table will be.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

- Duplicate comment -

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Fixed

This has been implemented.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: *Viewing* the sitemap should not also *update* it. » Viewing the sitemap should not also update it

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

As result of rolling back the code, the sitemap is updated when it is viewed.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Closed (fixed) » Active
Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Postponed

I don't see this for 1.x version. Will postpone until after 6.x-1.0 is released.

Anonymous’s picture

Anonymous (not verified) commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Postponed » Fixed

Since this is being addressed by 2.x I'll leave it there.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.