After updating from 7.x-2.3 to 7.x-2.4 I am no longer able to save Inclusion options for new nodes created or existing nodes on the site. I am able to save Inclusion options for the Content Type successfully. Error in the log appears to be related to Field Collection module? However it works fine when rolling back to the .3 version.

Notice: Undefined index: in FieldCollectionItemEntity->fetchHostDetails() (line 315 of \sites\all\modules\contrib\field_collection\field_collection.entity.inc).

Steps to duplicate:
Content > Add Content > Select Content Type > Scroll Down > Select XML Sitemap Section > Change Inclusion setting to 'Excluded' > Save > Edit Content Again > Inclusion remains at 'Default' setting for that Content Type.

After a rollback to 7.x-2.3 content is saving as intended.

I was able to test this and the error continues to occur in a clean drupal install.

Steps: Install latest version of Drupal 7: 7.59
Install the latest version of xmlsitemap: 7.x-2.4

after installing and enabling all modules:
content > add content > article > Title: Test Content: Test ect. > Set XML Sitemap to 'Included' > Save
Got back to re-edit the content and the XML Sitemap Inclusion continues to read 'Default'

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrgoodfellow created an issue. See original summary.

mrgoodfellow’s picture

Issue summary: View changes
mrgoodfellow’s picture

I tried this with the latest dev branch as well and I have the same issue. Tried to check the debuggin with devel but I don't see where this value is being stored.

In the DB, the xmlsitemap table is EMPTY until I use version 7.x-2.3, make changes to Inclusion option and then the page appears on that table list. When upgrading to 7.x-2.4 no changes are made to the xmlsitemap table.

mrgoodfellow’s picture

Issue summary: View changes
mrgoodfellow’s picture

Issue summary: View changes
mrgoodfellow’s picture

Issue summary: View changes
Rory Downes’s picture

I have found the same issue. I took a quick look at the code but it is beyond my skills but I am happy to help test a patch.
Rory

rjw’s picture

I can confirm also. Installed 7.x-2.4 on an existing site and enabled XML sitemap and XML sitemap node. Set to "Included" with priority 0.5 on a content type. Tried to set a page of that type to "Excluded." The setting for that page is not saved, it reverts to Default.

pandaski’s picture

This issue is brought from the recent security release

The value is saved in the Drupal queue instead of an updating immediately. Therefore, the node cannot be updated until the CRON job.

A quick fix is saving the sitemap link initially and this link will be updated during CRON.

/**
 * Implements hook_node_update().
 */
function xmlsitemap_node_node_update(stdClass $node) {
  // Node access can not be accurately determined in hook_node_update() because
  // node grants have not yet been written to the table, so we defer creation of
  // a sitemap link and process during cron.
  $queue = DrupalQueue::get('xmlsitemap_node');
  $queue->createItem($node->nid);
}
pandaski’s picture

Based on the above findings, I would like to propose an "xmlsitemap_link_presve" function in favour of the recent security release.

This function saves a sitemap link with access = 0 initially, then the Drupal CRON will update the sitemap link properly

At last, the link can be updated either after exceeding Minimum sitemap lifetime or triggering "rebuild links" through "/admin/config/search/xmlsitemap/rebuild"

pandaski’s picture

Status: Active » Needs review
mrgoodfellow’s picture

Hi Joseph,

Thank you for your time and response. Patch #10 appears to be working successful in both my current development environment and my clean Drupal install. I will continue to test and let you know if I experience any further issues.

mrgoodfellow’s picture

Status: Needs review » Reviewed & tested by the community

I have completed testing with my team and we have confirmed this resolves the bug and have not found any other functionality issues. This patch should be ready to port to the next branch.

Rory Downes’s picture

Status: Reviewed & tested by the community » Needs review

Thanks guys. That works for me.

tobybellwood’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC as per comment in #14

Rory Downes’s picture

Ah, I spoke too soon. In my last comment, I had only tested that the field could be updated. Having included two pages they do not appear in the sitemap. I ran the cron manually and when that did not work used xmlsitemap's Rebuild links function. The two pages are still not in the sitemap.

Did I do something wrong or is there a new bug?
Rory

pifagor’s picture

Status: Reviewed & tested by the community » Needs review

Setting back to Needs review as per comment in #16

pandaski’s picture

@Rory Downes

Can you kindly provide more configurations or steps so that I can duplicate from my local environment?

I've attached a few screenshots regarding my local environment testing

1. /admin/config/search/xmlsitemap/settings

Minimum sitemap lifetime - this is why the CRON job not working. You may want to reset it to "No minimum" for testing

2. When you rebuild the links

If "Save and restore any custom inclusion and priority links." checkbox is on can make any different results

pandaski’s picture

This patch adds a variable_set that marking the sitemaps as needing regeneration.

/**
 * Presave a sitemap link.
 *
 * @param array $link
 *   An array with a sitemap link.
 * @param array $context
 *   An optional context array containing data related to the link.
 */
function xmlsitemap_link_prsave(array $link, array $context = []) {
  // Force link access to 0 in presave.
  $link['access'] = 0;

  // Allow other modules to alter the sitemap link presave.
  drupal_alter('xmlsitemap_link_presave', $link, $context);

  // Save or update a sitemap link which will be overwritten in Drupal cron job.
  xmlsitemap_link_save($link, $context);

  // Mark the sitemaps as needing regeneration.
  variable_set('xmlsitemap_regenerate_needed', TRUE);
}
pandaski’s picture

webservant316’s picture

is this a reason to not upgrade to 2.4?

tobybellwood’s picture

There have been a number of issues logged since the 2.4 release (which in itself was just a patch, not a 7.x-2.x rollup) - one of which has already been committed (https://www.drupal.org/project/xmlsitemap/issues/2986847).

You should read through them, and assess your susceptibility to them.

Rory Downes’s picture

@Joseph Zhao Sorry for the delay. I had forgotten about the Minimum sitemap lifetime setting, changing that to 'No minimum' for testing worked! So Patch 10 did work.

I also tried patch 19, which I believe is intended to override this setting but if so that does not work as intended, and apears to have no impact. I am not sure however, why that is needed.

Anyway all good for me now.

Thanks for your efforts.
Rory

afinnarn’s picture

@tobybellwood I only see two issues listed under the 7.x-2.4 release. Other than this issue and the one you linked to, do you have a link to any other issues caused by the recent release?

sjerdo’s picture

Status: Needs review » Needs work

Some review comments:

  1. +++ b/xmlsitemap.module
    @@ -599,6 +599,28 @@ function xmlsitemap_link_load_multiple(array $conditions = array()) {
    +  // Force link access to 0 in presave.
    +  $link['access'] = 0;
    

    This could use a comment explaining why the access is set to 0.

  2. +++ b/xmlsitemap_node/xmlsitemap_node.module
    @@ -85,6 +85,9 @@ function xmlsitemap_node_node_insert(stdClass $node) {
    +  // Save a sitemap link which will be overwritten by the Drupal queue later.
    +  $link = xmlsitemap_node_create_link($node);
    +  xmlsitemap_link_prsave($link, array($link['type'] => $node));
       // Node access can not be accurately determined in hook_node_update() because
       // node grants have not yet been written to the table, so we defer creation of
       // a sitemap link and process during cron.
    

    Can these two comments be combined or could you add that the link is saved with revoked access until the node permissions are checked in the cron?

  3. +++ b/xmlsitemap.module
    @@ -599,6 +599,28 @@ function xmlsitemap_link_load_multiple(array $conditions = array()) {
    +function xmlsitemap_link_prsave(array $link, array $context = []) {
    

    Newly added hooks should be included in the [module].api.php file for the module.

  4. +++ b/xmlsitemap.module
    @@ -599,6 +599,28 @@ function xmlsitemap_link_load_multiple(array $conditions = array()) {
    +function xmlsitemap_link_prsave(array $link, array $context = []) {
    

    This module supports PHP 5.3+. Since short array syntaxis is included in PHP since 5.4, the long array syntaxis should be used.

  5. +++ b/xmlsitemap.module
    @@ -599,6 +599,28 @@ function xmlsitemap_link_load_multiple(array $conditions = array()) {
    +function xmlsitemap_link_prsave(array $link, array $context = []) {
    

    Typo: xmlsitemap_link_presave

sjerdo’s picture

Priority: Normal » Major
pandaski’s picture

Thanks, @Rory Downes @sjerdo for the feedback

Based on the advice, the revised patch:

1. The newly added hook is included in the xmlsitemap.api.php file.
2. The long array syntaxis is used for PHP 5.3+
3. The typo is fixed.
4. Revised comments

sjerdo’s picture

Seems good.
Adding the tests fix of #2989571: Fix tests for 7.x-2.x-dev branch to check if tests pass with this patch.

sjerdo’s picture

Status: Needs review » Reviewed & tested by the community

RTBC for patch #27 since all tests pass.

saurabh-chugh’s picture

Tested the patch #28 is working.

pifagor’s picture

Added users

  • pifagor committed 8d33ff9 on 7.x-2.x authored by Joseph Zhao
    Issue #2987125 by Joseph Zhao, sjerdo, mrgoodfellow, Rory Downes,...
pifagor’s picture

Version: 7.x-2.4 » 7.x-2.x-dev
Status: Reviewed & tested by the community » Fixed
Kasey_MK’s picture

After installing the dev branch with this commit, I agree that I can save per-node XML Sitemap settings.

However, those settings are not showing in the node's variables using Devel.

Setting those values programmatically appears to work, using...

$node->xmlsitemap['status'] = 0;
$node->xmlsitemap['status_override'] = 1;

...to exclude a node from the sitemap, for example.

Why don't those values show up on the Devel tab?

mrgoodfellow’s picture

After applying this patch to production and scheudle cron run, cron began to fail and multiple errors in the logs:

<code>Warning: Cannot modify header information - headers already sent by (output....

Recoverable fatal error: Argument 1 passed to xmlsitemap_node_create_link() must be an instance of stdClass, boolean given....

Cron also continue to fail.
This continued to happen until I manually rebuilt the sitemap files which created an error but triggered cron to stop running (in a loop)

It looks like the issue is related to this:
https://www.drupal.org/project/xmlsitemap/issues/2986847#comment-12722233

Where a node is both created and deleted before running cron.

I have rolled all updates back to 7.x-2.3 as 7.x-2.4 critical update does not appear to be stable at this time.

pifagor’s picture

Hello @mrgoodfellow
dev version (7.x-2.x-dev) has problems?

Status: Fixed » Closed (fixed)

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

mrgoodfellow’s picture

Hi pifagor, sorry I was referring to the 7.x-2.4 critical update release which introduced new critical issues. Wondering when this patch or the latest dev update will be pushed out as 7.x-2.5

darrell_ulm’s picture

Quick note: I believe the opposite is now happening for version 2.6:

Included Setting Switches to Excluded for Nodes, only Default Setting can Switch to Included if that is the Default

It looks like only default and excluded settings work. If setting a node to "Included" it switches to "Excluded."