Currently xmlsitemap uses only the node language to generate URLs and ignores nodes that have been translated with entity translation.

CommentFileSizeAuthor
#185 interdiff-1481798-177-185.txt518 bytesjames.williams
#185 xmlsitemap-add_support_for_entity_translation-1481798-185.patch19.86 KBjames.williams
#177 interdiff-1481798-172-177.txt8.68 KBjames.williams
#177 xmlsitemap-add_support_for_entity_translation-1481798-177.patch19.73 KBjames.williams
#172 interdiff.txt1.21 KBsergeimalyshev
#172 xmlsitemap-add_support_for_entity_translation-1481798-172.patch14.07 KBsergeimalyshev
#164 interdiff_142-164.txt605 bytesbojan_dev
#164 xmlsitemap-add_support_for_entity_translation-1481798-164.patch14.24 KBbojan_dev
#161 interdiff_142-161.txt563 bytesbojan_dev
#161 xmlsitemap-add_support_for_entity_translation-1481798-161.patch14.24 KBbojan_dev
#159 interdiff_142-159.txt563 bytesbojan_dev
#159 xmlsitemap-add_support_for_entity_translation-1481798-159.patch97.98 KBbojan_dev
#157 xmlsitemap-add_support_for_entity_translation-1481798-156.patch12.21 KBbojan_dev
#155 xmlsitemap-add_support_for_entity_translation-1481798-155.patch12.2 KBbojan_dev
#142 interdiff_139-142.txt203 byteshctom
#142 xmlsitemap-add_support_for_entity_translation-1481798-142.patch14.21 KBhctom
#137 xmlsitemap-add_support_for_entity_translation-1481798-137.patch14.16 KBphjou
#131 xmlsitemap-add_support_for_entity_translation-1481798-131.patch13.99 KBothermachines
#131 _interdiff.txt7.54 KBothermachines
#130 interdiff.txt7.54 KBothermachines
#130 xmlsitemap-add_support_for_entity_translation-1481798-130.patch13.99 KBothermachines
#129 xmlsitemap-add-support-for-entity-translation-1481798-129.patch12.69 KBorlando.thoeny
#128 add_support_for_entity_translation-1481798-128.patch14.68 KBzread
#118 add_support_for_entity_translation-1481798-118.patch13.02 KBmdixoncm
#116 add_support_for_entity_translation-1481798-116.patch13.9 KBmarco-s
#114 add_support_for_entity_translation-1481798-114.patch13.89 KBfprevos2
#109 add_support_for_entity_translation-1481798-109.patch13.34 KBciss
#107 add_support_for_entity_translation-1481798-107.patch12.86 KBpavel.zheldak
#93 interdiff-1481798-90-93.txt1.47 KBkalistos
#93 add_support_for_entity_translation-1481798-93.patch13.32 KBkalistos
#90 interdiff-1481798-76-90.txt3.53 KBkalistos
#90 add_support_for_entity_translation-1481798-90.patch13.14 KBkalistos
#76 interdiff-1481798-71-76.txt1.71 KBsergei_brill
#76 add_support_for_entity_translation-1481798-76.patch12.96 KBsergei_brill
#71 interdiff-1481798-64-71.txt1.33 KBvasike
#71 add_support_for_entity_translation-1481798-71.patch11.88 KBvasike
#68 updated-patch.patch11.76 KBLangley
#64 interdiff-1481798-59-64.txt1.18 KBDeFr
#64 add_support_for_entity_translation-1481798-64.patch11.49 KBDeFr
#59 interdiff-1481798-57-58.txt626 bytesDeFr
#59 add_support_for_entity_translation-1481798-58.patch11.49 KBDeFr
#57 interdiff-1481798-47-57.txt3.7 KBDeFr
#57 add_support_for_entity_translation-1481798-57.patch11.49 KBDeFr
#56 xmlsitemap-add_support_for_entity_translation-1481798-56.patch11.45 KBPascalAnimateur
#52 interdiff-1481798-47-52-do-not-test.txt2.34 KBKristen Pol
#52 xmlsitemap-add_support_for_entity_translation-1481798-52.patch13.62 KBKristen Pol
#47 interdff-1481798-46-47.txt515 bytessergei_brill
#47 add_support_for_entity_translation-1481798-47.patch11.63 KBsergei_brill
#3 xmlsitemap_entity_translation.patch2.23 KBromualdbrisson
#7 xmlsitemap-support_entity_translation-1481798.patch5.84 KBe.escribano
#11 interdiff-1481798-7-11-do-not-test.diff7.34 KBdas-peter
#11 xmlsitemap-support_entity_translation-1481798-11.patch7.98 KBdas-peter
#21 interdiff-1481798-11-21-do-not-test.diff1.23 KBdas-peter
#21 xmlsitemap-support_entity_translation-1481798-21.patch8.18 KBdas-peter
#23 xmlsitemap-support_entity_translation-1481798-23.patch9.4 KBDeFr
#23 interdiff-1481798-23.txt3.19 KBDeFr
#25 interdiff-1481798-25.txt804 bytesDeFr
#25 xmlsitemap-support_entity_translation-1481798-25.patch9.4 KBDeFr
#31 xmlsitemap_doesn_t_work-1481798-31.patch11.03 KBcolan
#34 add_support_for_entity_translation-1481798-34.patch11.48 KBcolan
#35 interdiff-1481798-34-35-do-not-test.diff1.48 KBdas-peter
#35 add_support_for_entity_translation-1481798-35.patch10.72 KBdas-peter
#45 add_support_for_entity_translation-1481798-45.patch11.46 KBsergei_brill
#46 add_support_for_entity_translation-1481798-46.patch11.62 KBDeFr
#46 interdiff-1481798-45-46.txt5.27 KBDeFr
#46 interdiff-1481798-35-45.txt1017 bytesDeFr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

das-peter’s picture

I just came across this issue. Atm. for me it looks like to support this we've to alter the database schema. Currently the xmlsitemap table has id and type as key and a row can have a single language.
But since with entity translation the id / type combination will be shared for multiple languages we loose the unique identifier.
And I'm not sure if the "simple" introduction of language is really sustainable.

Btw. for me this issue is related to following other issues:
#1461670: Generalize to work with any Entity Type
#891696: Consistently use link, bundle, and entity loaders everywhere

All of them are mainly about to better integrate with the "core" entity system.

marcoka’s picture

romualdbrisson’s picture

Here is a patch for a simple solution on this issue.
You have to rebuild links.

hefterbrumi’s picture

This doesnt work for me

muschpusch’s picture

i started working on entity translation support over here: #1370394: Set correct language for url()

hefterbrumi’s picture

great news indeed

e.escribano’s picture

I needed the implementation for entity_translation myself so I made a patch that includes also support for taxonomy terms.

EDIT:

Didn't notice that the alter in the install file gave me problems when a node was saved. It is not a complete patch yet.

e.escribano’s picture

matsbla’s picture

titouille’s picture

#7 seems to be broken with last xmlsitemap version. follow code must be edited in xmlsitemap_link_save function (in xmlsitemap.module) :

  // Save the link and allow other modules to respond to the link being saved.
  if ($existing) {
-    drupal_write_record('xmlsitemap', $link, array('type', 'id'));
+    drupal_write_record('xmlsitemap', $link, array('type', 'id', 'language'));
    module_invoke_all('xmlsitemap_link_update', $link, $context);
  }
  else {
    drupal_write_record('xmlsitemap', $link);
    module_invoke_all('xmlsitemap_link_insert', $link, $context);
  }

Because patch add language as primary key, drupal_write_record must pass 'language' as primary key to work correctly.

das-peter’s picture

Status: Active » Needs review
FileSize
7.34 KB
7.98 KB

Re-rolled #7, incorporated #10 and centralized the translation handling.
New there's xmlsitemap_get_entity_languages() which fetches the available languages.
This new function just returns the languages of published translations. It also uses the translation handlers to get the information instead accessing the data directly - this should be safer.
And last but not least it just handles entities with entity_translation enabled.

J-Lee’s picture

#11 is working for me with entity translated nodes/fields.
Thank you.

J-Lee’s picture

It seems, I was to fast at #12 ....
If found that nodes without translations are added to the sitemap too.

I have two languages (en and de) with a node which is only available in en.
At the sitemap there is an entry for /en/[node-alias] which is ok.
But there is an entry for /de/node/[nid] too.

I'm using entity translation for title-field and body.

ph08n1x’s picture

#11 worked for me too, thank you so much! :D

in regards to showing languages that don't have translations, luckily all my pages are translated in all three of my languages... hmmm mabe there is a way to check the state of node if it is published for that language...?

ph08n1x’s picture

Hi J-Lee perhaps this will help you (sorry about not supplying a patch but super busy today!):

@@ -1629,7 +1629,7 @@ function xmlsitemap_get_entity_languages($entity_type, $entity, $default_languag
         $languages = array();
         $translations = $handler->getTranslations();
         foreach ($translations->data as $translation_lang => $translation_data) {
-          if (!empty($translation_data['status'])) {
+          if (!empty($translation_data['status']) && $translation_data['status'] == 1) {
             $languages[$translation_lang] = $translation_lang;
           }
         }

I saw that the if statement only checks if status is empty it does not check if status is equal to 1 (i.e. is the entity published).

Although I'm not sure if that is checking if the entity translation is published or if it's just checking if the default language node is published... Give it a go tho :D

das-peter’s picture

@ph08n1x empty() should be fine:

No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false.
....
The following things are considered to be empty:

"" (an empty string)
0 (0 as an integer)
0.0 (0 as a float)
"0" (0 as a string)
NULL
FALSE
array() (an empty array)
$var; (a variable declared, but without a value)

This means 1, TRUE, 'published' etc. are considered as published. And as far as I know status is here an int or bool.
Edit: Fixed formatting

J-Lee’s picture

I found the issue from #12 ...
Because I used the node_translation_sitemap (from #9) before, I have multible 'node_translated_de' entries at the 'type' column in the xmlsitemap table.

I had to trunkate the xmlsitemap table (without the first row, which is the front page) and rebuilding the links.

Now it is working.

@ph08n1x: Thank you for your suggestion.
Unpublished translations are not displayed, so #15 is not needed, I think. But it will be fine if someone can confirming it.

Edit: das-peter has confirmed it a few minutes before :)

MXT’s picture

Sorry guys,

a banal question: applying patch in #11 is enough or I have to install XML_sitemap_internationalization submodule also?

Thank you very much

J-Lee’s picture

XML sitemap internationalization adds one sitemap for each language. E.g. www.example.com/en/sitemap.xml and www.example.com/de/sitemap.xml. If you only want one sitemap with multiple languages, patch #11 is enough.

DeFr’s picture

Status: Needs review » Needs work

Moving to NW, because there's a few things that seems wrong with regards to the schema handling.

This is adding a xmlsitemap_node_update_6202 update function, to tweak the xmlsitemap table. That update function number seems off for a Drupal 7 module (the update will still run because the last update in that file is 6201, so every Drupal 7 site out there have a schema_version of 6201 for xmlsitemap_node), but more importantly, xmlsitemap_node isn't the mode that provides the xmlsitemap table in the first place.

I think instead, the module should add an update function to xmlsitemap.install, and also tweak the default primary key defined in xmlsitemap_install to handle new installations.

das-peter’s picture

Address points from #20.

J-Lee’s picture

Patch #21 is working on a clean Drupal 7.37 installation with latest title- and entity_translation module.
Thank you das-peter.

DeFr’s picture

Thanks das-peter, the schema part does look a lot better now. Sadly found a bunch of others issues while testing various things that I had not in the first review :-(

First problem is due to the change in the $existing in xmlsitemap_link_save being conditional on entity translation activation, while the primary key and the drupal_write_record are not. That means that if you don't have entity translation enabled and the entity language changes, you won't get any update for this sitemap link anymore.

(To reproduce that issue: install Drupal core, its bundled Locale and Translation modules, install XML Sitemap and XML Sitemap Node, add another language and change the article bundle multilingual settings from Disabled to Enabled while adding that content type to your site map. Then add an article node in English, save it, and change the node language to something else… No update to the xmlsitemap table anymore)

Attached patch tries to fix the problem by always using the same key while checking for existence and updating, which fix that problem, but I'm not sure that's completely correct either.

I think that's the only problem for people not using entity translation ; moving to problems specific to entity translation now:

  1. There's an implicit assumption that having the Entity Translation enabled means that it'll be used to managed the entities. That's not mandatory though, and should be checked with entity_translation_enabled otherwise you get stale links in the table ; added that to the patch.
  2. There's a problem with translation deletion ; right now there's nothing to delete the corresponding link from the sitemap.
  3. Finding that lead me to xmlsitemap_link_delete, which should also probably be tweaked to accept an optional language parameter now that this is part of the primary key ? As it is now, it's misleading, because its name implies that it'll delete a single link, while in fact it can deletes a bunch of them, and the code is adding a link_delete_mutliple call which will target a single link
  4. _xmlsitemap_check_changed_link is also doing a custom query to load a link that's not taking the language into account
  5. At that point I think we need a module maintainer to chime in, to have an idea of how invasive the acceptable changes can be. There's a lot of code in there that expects the primary key to be (type, id), and changing them all is going to be time consuming.

maxplus’s picture

Hi,

First of all: thanks for the great work done already!

I have just tested the patch from #23 and I'm getting this error:

Fatal error: Cannot redeclare xmlsitemap_node_entity_translation_delete() (previously declared in /.../sites/all/modules/xmlsitemap/xmlsitemap_node/xmlsitemap_node.module:90) in /.../sites/all/modules/xmlsitemap/xmlsitemap_taxonomy/xmlsitemap_taxonomy.module on line 158

Then I tested the patch from #21, and that runs fine.
=> Now I have three correct xml-sitemaps, one for every language (Dutch, French and English)

Using Drupal 7.36, XML sitemap 7.x-2.2+3-dev and Entity Translation 7.x-1.0-beta4

DeFr’s picture

The fatal error in #24 was due to a bad copy & paste between xmlsitemap_node and xmlsitemap_taxonomy, sorry for that. Attached fixed patch + interdiff.

gstout’s picture

Latest patch worked great. We should get this into a dev release.

Neograph734’s picture

The patch appears to be working nicely, and applied flawless. It would indeed be nice if this could get committed. Should we test more or is this RTBC?

colan’s picture

Priority: Normal » Major
Issue tags: +i18n, +Release blocker

I'll be testing this very shortly. Seems to apply to the latest dev.

colan’s picture

Issue tags: -Release blocker

Removing tag.

colan’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good and works well. It also appears to solve the problem described in #1370394: Set correct language for url(). Here are the steps I took to get this up & running:

  1. Enable the XML Sitemap Internationalization submodule.
  2. Apply the patch from #25.
  3. Delete the default sitemap.
  4. Add a new sitemap for each language.

Can anyone comment on #2220641: Merge this function back to xmlsitemap module?? Now that we have this code, is that module obsolete?

colan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.03 KB

Whenever the XML Sitemap Internationalization submodule (xmlsitemap_i18n) is enabled, sitemaps will now be created automatically for each language. This patch is the same as the previous one, except that I've added xmlsitemap_i18n_install().

colan’s picture

Title: xmlsitemap doesn't work with nodes translated with entity translation (and title_field) » Add support for Entity Translation & Title modules

Better title.

Neograph734’s picture

Status: Needs review » Needs work

Colan, your patch seems to be working nice. There is one thing I am not completely sure about;

If I enable the patched module while i18n is configured and in place, there are two sitemaps created for the english (default language). One named English and one named Default. The sitemaps for the other languages are fine.
Though this seems harmless and one of both can easily be deleted, it might be causing problems when the system tries to write the same file twice.

When I changed the i18n default language, the sitemap default language followed as expected. Therefor I suppose we should attempt to remove the default language, rather then to prevent the English (or other language) from being created. If one was to change the i18n default language later the same problem would occur with duplicate languages and English would even be missing.

You think you could add that to the patch?

colan’s picture

Status: Needs work » Needs review
FileSize
11.48 KB

Ask and you shall receive! Added a DB call to delete the default sitemap so it doesn't cause any confusion. It didn't seem to conflict with the new default language sitemap, but I agree, it's better to remove it.

das-peter’s picture

I think this looks pretty good now.
Just gave it another visual review and just found some minor coding style nit-picks.
Patch updated.
As I'm one of the people fiddling with the patch I probably shouldn't RTBC this :)

colan’s picture

Thanks das-peter. I noticed those, but forgot to fix them. I better not RTBC this either. :)

For anyone testing this, all you should have to do is:

  1. Apply the patch.
  2. Enable the XML Sitemap Internationalization module (xmlsitemap_i18n).
  3. Run cron one or more times (some links are generated each cron run).
Dave Reid’s picture

Status: Needs review » Needs work

Some overall review:

  • I think it's kind of strange that we're encouraging people to install xmlsitemap_i18n when they may not actually want to be using the i18n module (and only entity_translation)? Do we want to provide support for per-language sitemaps in the base module if locale.module is enabled?
  • We should add an optional langcode parameter to xmlsitemap_link_delete() instead of using xmlsitemap_link_delete_multiple(). Then we could just implement xmlsitemap_entity_translation_delete() instead of requiring each submodule to implement the hook.
  • Need a change record for 'language' supporting an array of langcodes instead of just a single langcode string. Functions like xmlsitemap_node_create_link() are public APIs and looks like this change would break existing implementations. Maybe it would be better to *add* $link['languages'] for multiple-language-per-link support instead.
Neograph734’s picture

Dave, pleaese be aware that though there is some overlap, entity translation is no replacement for i18n as stated on the module page. It is just another way of translating content. Language selectors and other aspects of i18n are still required to actually use entity translation. So I assume the i18n requirement is not much of an issue.

The other points are worth thinking about.

colan’s picture

To make it clear that xmlsitemap_i18n isn't only for the i18n module (I was confused about that myself when I first got into this issue), how about if we change the module description from Enables multilingual XML sitemaps to Enables XML sitemaps for multiple languages via the Locale module, used by Internationalization and Entity Translation.

I'm fine with moving this stuff into the base module - not sure why it was split off in the first place.

Dave Reid’s picture

Simple, Entity Translation didn't exist yet. And without i18n there was no way to only have certain language content on certain domains, and not all content always on every domain.

MXT’s picture

XML_sitemap_internationalization submodule should not be mandatory.

I'm not using it in my use case (see #18 and #19).

nlambert’s picture

#35 works for me

Maybe out of scope for this issue, but one thing that seems to be missing is a way to add a language neutral sitemap that could point to language specific sitemaps. Or maybe the only way to do this is to actually put a sitemap.xml in the Drupal root.

E.g.:
/sitemap.xml # With links to language specific sitemaps:

/fr/sitemap.xml
/pt/sitemap.xml
/en/sitemap.xml
...

vimokkhadipa’s picture

#35 works for me too
I applied the patch # 35, and it worked perfectly.
xmlsitemap Versión: 7.x-2.2+3-dev

pipicom’s picture

#35 works for me also
xmlsitemap version: 7.x-2.2

sergei_brill’s picture

The patch works well for me except one thing. After I applied the patch, I tried to disable xmlsitemap_i18n and enable it again. I got below error message:
exception 'PDOException' with message 'SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3QuYVMoAVDdMUmwuEdFhwSFxLtcBNDrxIy91p4VzGdY' for key 'PRIMARY'' in [error]
/home/sergei/web/tours/site/includes/database/database.inc:2171
Stack trace:
#0 /home/sergei/web/tours/site/includes/database/database.inc(2171): PDOStatement->execute(Array)
#1 /home/sergei/web/tours/site/includes/database/database.inc(683): DatabaseStatementBase->execute(Array, Array)
#2 /home/sergei/web/tours/site/includes/database/mysql/query.inc(36): DatabaseConnection->query('INSERT INTO {xm...', Array, Array)
#3 /home/sergei/web/tours/site/sites/all/modules/contrib/xmlsitemap/xmlsitemap_i18n/xmlsitemap_i18n.install(35): InsertQuery_mysql->execute()
#4 [internal function]: xmlsitemap_i18n_install()
#5 /home/sergei/web/tours/site/includes/module.inc(905): call_user_func_array('xmlsitemap_i18n...', Array)
#6 /home/sergei/web/tours/site/includes/module.inc(477): module_invoke('xmlsitemap_i18n', 'install')
#7 /home/sergei/.composer/vendor/drush/drush/commands/core/drupal/environment_7.inc(143): module_enable(Array)
#8 /home/sergei/.composer/vendor/drush/drush/commands/pm/pm.drush.inc(1120): drush_module_enable(Array)
#9 [internal function]: drush_pm_enable('xmlsitemap_i18n')
#10 /home/sergei/.composer/vendor/drush/drush/includes/command.inc(359): call_user_func_array('drush_pm_enable', Array)
#11 /home/sergei/.composer/vendor/drush/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#12 [internal function]: drush_command('xmlsitemap_i18n')
#13 /home/sergei/.composer/vendor/drush/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#14 /home/sergei/.composer/vendor/drush/drush/lib/Drush/Boot/BaseBoot.php(62): drush_dispatch(Array)
#15 /home/sergei/.composer/vendor/drush/drush/drush.php(70): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#16 /home/sergei/.composer/vendor/drush/drush/drush.php(11): drush_main()
#17 {main}

I guess hook_uninstall should be included to xmlsitemap_i18n.install to remove contexts?

DeFr’s picture

Worked on the review comments in #37 to try to keep moving this forward :-) Attaching interdiff and patch, plus missing interdiff for #45 that was posted in between.

In details, response to the various points of #37 below:

  1. I think changing xmlsitemap_i18n module description is the way to go ; not sure about moving that to XMLSiteMap itself. Moving it to the core with the corresponding install hook, then MXT use case in #18 (have one site map with all links in all language) become trickier (the default sitemap would get deleted, replaced by site map per language. You'd then have to delete those sitemaps, and re-create a new one without context)
  2. Changed that ; not completely sure about it though, because untill this patch, each module providing links for XMLSitemap was responsible of deleting them as needed, except for bundle renames (e.g. there's no hook_entity_delete in xmlsitemap.module)
  3. Makes sense to minimize the BC break. As an additional bonus, this allow caller to distinguish the source language and the set of available translations if they want to do so.
sergei_brill’s picture

Small fix for last patch

DeFr’s picture

@sergei_brill: Thanks for that and sorry for that typo :-)

Sakhmed’s picture

I have two languages enabled on my site English and Russian. Have several nodes that were originally created in English and then translated into Russian. Applied patch #47. Patch has been applied smoothly. Have two sitemaps for Russian and English. I can see all translated nodes in English sitemap but no translated nodes in Russian sitemap.

Using xmlsitemap-7.x-2.2+3-dev

Update

By some reasons translated nodes show up in actual xml file located in public directory. They are not showing in the sitemap that is available for viewing via drupal admin interface

sergei_brill’s picture

@Sakhmed Make sure you did database update (/update.php or drush updb). And try to rebuild sitemaps here /admin/config/search/xmlsitemap/rebuild

Kristen Pol’s picture

Works great! Thanks for the hard work. I found one minor typo:

+++ b/xmlsitemap.module
@@ -663,11 +673,17 @@ function xmlsitemap_link_update_multiple($updates = array(), $conditions = array
+ *  An optionnal language code for the link that should be deleted.

optionnal => optional

Kristen Pol’s picture

I found an issue where the sitemaps were not being updated via cron when entity translation was done but the source node was untouched. The sitemaps would only get updated correctly when running the rebuild links option manually. To reproduce this issue:

  1. Apply patch from #47
  2. Enable 2 languages and configure xmlsitemap, xmlsitemap_node, and xmlsitemap_i18n for the 2 languages, e.g. English and Spanish
  3. Enable and configure entity_translation for a content type, e.g. article
  4. Create node for that content type in one language, e.g. English
  5. Run cron
  6. Check the sitemap for that language => it should have the node listed
  7. For that node, click "Translate" tab
  8. Click "add" for the other language, e.g. Spanish
  9. Translate the node and save
  10. Run cron
  11. Check the sitemap for that language => the node will be missing

I have updated the patch from #47 and added some code to xmlsitemap_i18n_query_xmlsitemap_generate_alter that will check if entity_translation is enabled and, if so, will use the entity_translation table for the query to find the nodes that need to be rebuilt. This seems like the logical place to add it but, if there are other suggestions, let me know.

I also fixed the minor typo noted in my previous comment (#51). Thanks!

phanosd’s picture

Hey all,

Recently had the issue where only "source" language nodes would appear on my sitemap. Patch #35 fixed this for me.

I have since however come across an undocumented issuewherever I try to create a new taxonomy term, or add translation to ANY taxonomy. Using xmlsitemap: 7.x-2.2+3-dev patch #35, 6 languages enabled

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ' 'en-cy', 'en-uk', 'en-sa', 'en-tn', 'en-my', 'en-ca', 'ru-ru', 'es-ar', 'es-mx'' at line 1: SELECT loc, access, status, lastmod, priority, changefreq, changecount, language FROM {xmlsitemap} WHERE type = :type AND id = :id AND language = :language_0, :language_1, :language_2, :language_3, :language_4, :language_5, :language_6, :language_7, :language_8, :language_9, :language_10, :language_11, :language_12, :language_13, :language_14, :language_15 LIMIT 0, 1; Array ( [:type] => taxonomy_term [:id] => 7 [:language_0] => en [:language_1] => en-cy [:language_2] => en-uk [:language_3] => en-sa [:language_4] => en-tn [:language_5] => en-my [:language_6] => en-ca [:language_7] => ru-ru [:language_8] => es-ar [:language_9] => es-mx [:language_10] => es-es [:language_11] => ar-sa [:language_12] => en-ng [:language_13] => en-id [:language_14] => id-id [:language_15] => pl-pl ) in xmlsitemap_link_save() (line 611 of /var/www/wwwtest.hiwayfx.com/sites/all/modules/xmlsitemap/xmlsitemap.module).

*EDIT*

Went to sitemap xlm module and deselected "node" from inclusion in sitemap. Rebuild links, problem has disappeared.

PascalAnimateur’s picture

I just tested #52 and it adds the original URL of the (untranslated) page to the translated sitemap.
For example, I create the "Test page" in english (/en/pages/test-page) and translate it to french as "Page de test" (/fr/pages/page-de-test), and my french sitemap has /en/pages/test-page instead of /fr/pages/page-de-test

Any ideas?

PascalAnimateur’s picture

Update: It seems the check for is_array($link['language']) in function xmlsitemap_node_node_update always returns FALSE ... perhaps it should check for index 'languages', which is an array?

PascalAnimateur’s picture

Update (again): It seems the problem is with the generation of the sitemap itself, not the individual links being inserted / updated in the xmlsitemap table.

By adding a language condition in function xmlsitemap_generate_chunk, my sitemaps only contain links in the proper language! (see line 175 in xmlsitemap.generate.inc)

Here's patch #52 re-rolled with this additional line...

DeFr’s picture

The _node_update and _taxonomy_term update do seems off, seems like I didn't convert them properly when switching to the 'languages' key in #46 ; sorry about that. Uploading a patch that fix that.

I think that explains part of the problem described in #52. Not sure about the query_alter introduced in there though: I see that they're a port of the strict mode below, but adding some node specific case to handle Entity Translation that aims to be fully generic seems unexpected.

I also think that this query_alter is the cause of the problem @PascalAnimateur encountered.

So, what I'm attaching is #47 + the typo fix mentionned in #50 + the fix to the update calls.

@Kristen Pol, @PascalAnimateur: Can you please test and confirm if this works for you ?

Status: Needs review » Needs work

The last submitted patch, 57: add_support_for_entity_translation-1481798-57.patch, failed testing.

DeFr’s picture

Forgot to git add the latest change before creating patches…

That one should be better.

DeFr’s picture

Status: Needs work » Needs review
chrisdarke42’s picture

#35 works for me too, I tried #52 and #56 but they were generating extra entries in the sitemaps for the wrong languages. Admittedly, I have not tested the taxonomy issue but if you don't need that, #35 seems to be good

DeFr’s picture

@chris.darke: Could you please try #59 ?

PascalAnimateur’s picture

@DeFr: I tested your patch and the french sitemap entry is not created for the node translation when I create it from the original english node.

Adding the query condition from my patch probably won't fix that, as I don't see the 'fr' entry for that translation in the database.

DeFr’s picture

Took the time to really test the patch I'm attaching this time, and everything seems OK. Running cron after adding a translation makes the link appear in the correct sitemap, deleting the translation makes it disappear, tested for node and taxonomy term. Sorry for the obvious typo in the patch above.

@PascalAnimateur If you can please confirm, it'd be more than welcome :-)

PascalAnimateur’s picture

Status: Needs review » Reviewed & tested by the community

@DeFr : Your patch works great for node content and taxonomy terms!
Marking this as RTBC, although more people should probably inspect / test this further.

bwaindwain’s picture

Yay! Patch #64 fixes xmlsitemaps with nodes and menus for my test site in 3 languages. Thx @DeFr

Neograph734’s picture

Seems to be working for me as well. Two languages and both site maps contain what they should contain.

Thanks!

Langley’s picture

Thanks for the patch it works great, except when trying to add taxonomy to sitemaps. The patch contains

xmlsitemap_taxonomy_taxonomy_term_update($term);

which should probably be

xmlsitemap_taxonomy_term_update($term);

?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 68: updated-patch.patch, failed testing.

Skin’s picture

Hello,
with #64, after applying the patch and after running update.php I have this error if I try to rebuild the sitemaps:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /site/batch?id=399&op=do StatusText: Internal Server Error ResponseText:

If i try with #68 I'm unable to apply the patch:

git apply -v updated-patch.patch
updated-patch.patch:10: trailing whitespace.
'primary key' => array('id', 'type', 'language'),
updated-patch.patch:18: trailing whitespace.

updated-patch.patch:19: trailing whitespace.
/**
updated-patch.patch:20: trailing whitespace.
* Add new primary key including the language.
updated-patch.patch:21: trailing whitespace.
*/
fatal: corrupt patch at line 33

vasike’s picture

Status: Needs work » Needs review
FileSize
11.88 KB
1.33 KB

There is an update for the #64 patch.
It seems the previous patches do not do anything about about for languages for entities not yet translated having "Enable language fallback" option set.
So we have these pages available for other languages than source/original but they do not get in/out of sitemaps.

This update also includes the fix about using xmlsitemap_taxonomy_taxonomy_term_update($term) in xmlsitemap_taxonomy_xmlsitemap_process_taxonomy_term_links() (see #68).

Skin’s picture

I reinstalled 7.x-2.x-dev and applied #64 and after #71, but now I don't see taxonomy terms in the localized sitemaps, they appear only in the main language.

joachim’s picture

Patch works great, though existing nodes that aren't showing in the sitemap aren't fixed by this until they've been edited.

jurgenhaas’s picture

Status: Needs review » Reviewed & tested by the community

Tested it and it works fine for me. In terms of updating all nodes, I have written a simple drush command doing this:

  foreach (node_load_multiple(FALSE) as $node) {
    xmlsitemap_node_node_update($node);
  }
joachim’s picture

Cool! I hadn't managed to work out the right API to call in this module to update its data.

Though that's not going to scale for large sites. Ideally, it should be a hook_update_N() which uses the batch functionality to process the nodes in chunks.

Also, we could do to process only the nodes that need it maybe? I reckon this should give us them:

SELECT * FROM `entity_translation`
WHERE entity_type = 'node'
AND source != `language` AND source != ''
GROUP BY `entity_id`

That's all the nodes that have a translation in addition to their source language.

sergei_brill’s picture

The last patch works great. But I needed to add an additional option for my case. I have original site in English and localized version of the site for Australia. On the site I have main content type which references to other content type (multiple entityreference field). Nodes of the second content type have no own pages (at least users don't see them). I don't want to translate each item of the second content type, but I don't want to show all items of first content type on AU version and don't want add them to sitemap.xml. Also I don't want translate bean blocks on the AU version. So, I need Language fallback to be enabled, but I don't want add untranslated content to sitemap.xml. Therefore I added additional variable xmlsitemap_language_fallback which could be configured on xmlsitemap settings page.

sergei_brill’s picture

Status: Reviewed & tested by the community » Needs review
DuneBL’s picture

Patch 71 doesn't work for me... after applying it, I get errors when rebuilding the map

jsacksick’s picture

I applied the patch #76 and it seems to work pretty well.
@DuneBL you need to run the database updates.

aschiwi’s picture

I successfully used the patch in #76. I have two languages enabled and add a few node types and vocabularies to the sitemap. There are about 200 entries per sitemap, so it's quite a small use case.

Thanks to everyone who helped with this patch <3

DuneBL’s picture

@jsacksick Thanks for the tip!
This time I have used the #76 patch and didn't got the errors.
Unfortunately, the sitemap is only made by the nodes in the original language, not the translated ones.

An example is better:

Node1: Created in FR, translated (field translation) in NL
=>/fr/node1 Will show up in the FR Sitemap but /nl/node1 will not appears in the NL Sitemap

The opposite is also true
Node2: Created in NL, translated in FR
=>/fr/node2 Will NOT shows up in the FR Sitemap but /nl/node2 will appears in the NL Sitemap

Yazzbe’s picture

I successfully applied patch #76 against version 2.3

  • Entity translated nodes are included in the correct language XML sitemap.
  • Language neutral nodes are all included in the default XML sitemap of the default language (they should only be listed in the default sitemap to prevent pushing duplicate content to Google of non-translated nodes).

PS: applying the patch manually was a pain. Hope to see this fix committed in a next release or so.

To me this looks RTBC.

suit4’s picture

Category: Feature request » Bug report

After upgrading a page (with entity translations) to the xmlsitemap 7.x-2.3, I got errors editing and resaving nodes.

Error message was:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '272-node-en' for key 'PRIMARY': UPDATE {xmlsitemap} SET subtype=:db_update_placeholder_0, loc=:db_update_placeholder_1, language=:db_update_placeholder_2, access=:db_update_placeholder_3, status=:db_update_placeholder_4, status_override=:db_update_placeholder_5, lastmod=:db_update_placeholder_6, priority=:db_update_placeholder_7, priority_override=:db_update_placeholder_8, changefreq=:db_update_placeholder_9, changecount=:db_update_placeholder_10 WHERE (type = :db_condition_placeholder_0) AND (id = :db_condition_placeholder_1) ; Array ( [:db_update_placeholder_0] => product [:db_update_placeholder_1] => node/272 [:db_update_placeholder_2] => en [:db_update_placeholder_3] => 1 [:db_update_placeholder_4] => 1 [:db_update_placeholder_5] => 0 [:db_update_placeholder_6] => 1464261898 [:db_update_placeholder_7] => 0.5 [:db_update_placeholder_8] => 0 [:db_update_placeholder_9] => 0 [:db_update_placeholder_10] => 0 [:db_condition_placeholder_0] => node [:db_condition_placeholder_1] => 272 ) in drupal_write_record() (line 7336 of /var/www/vhosts/olfasense.com/httpdocs/includes/common.inc).

Like yazzybe I applied patch #76 against 7.x-2.3 which fixed the constraint problem.

Applying the patch was easy using
patch -p 1 < add_support_for_entity_translation-1481798-76.patch

othermachines’s picture

#76 works well for me, thanks very much. Like @sergei_brill, I also don't want untranslated content added, so I'm pleased to have this additional setting.

After applying the patch be sure to run update,php (or drush updb).

milopca’s picture

Assigned: Unassigned » milopca
milopca’s picture

Assigned: milopca » Unassigned
d.sibaud’s picture

#76 works like a charm, good for a commit

jamsilver’s picture

Status: Needs review » Reviewed & tested by the community

#76 works well

The last submitted patch, 35: interdiff-1481798-34-35-do-not-test.diff, failed testing.

kalistos’s picture

I tried patch #76 and found some problems with it.
Disabling "This translation is published" flag for some translation doesn't remove node from Xmlsitemap with cache clearing.

So I have updated patch and interdiff.

kalistos’s picture

Status: Reviewed & tested by the community » Needs review
gonssal’s picture

Status: Needs review » Reviewed & tested by the community

Just tested #90 and everything seems to work fine. Update hook for PK worked too.

Drupal Commerce site with Entity Translation and fairly big number of products and related taxonomies. Indexed pages, blog posts, menu items and of course, products.

Moving to RTBC again.

kalistos’s picture

I have found that my previous patch doesn't work correctly with programmaticaly update. Source of this issue is in function xmlsitemap_link_load from function xmlsitemap_node_create_link must take $language variable also, if we need to add entity translation support by correct way. So I think we need to rework all places using xmlsitemap_link_load function with adding it.

But now I'm uploading patch that fixes this issue, and it looks it works correctly. But it isn't the best way.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 93: add_support_for_entity_translation-1481798-93.patch, failed testing.

candelas’s picture

Patch #76 works perfect. Thanks a lot. I have uninstalled xmlsitemap, apply the patch and then install and configure. Now I have the maps in 4 languages using the node and menus modules.

Skin’s picture

Any news? Is this bug still present?

saile’s picture

Tried the patches but when rebuilding the sitemap I get mysql contraint errors
Er is een AJAX HTTP fout opgetreden. HTTP-resultaatcode: 500 Debug informatie volgt. Pad: /batch?id=297&op=do Statustekst: Service unavailable (with message) Antwoordtekst: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '23636-node' for key 'PRIMARY': INSERT INTO {xmlsitemap} (id, type, subtype, loc, language, access, status, status_override, lastmod, priority, priority_override, changefreq, changecount) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12); Array ( [:db_insert_placeholder_0] => 23636 [:db_insert_placeholder_1] => node [:db_insert_placeholder_2] => veld [:db_insert_placeholder_3] => node/23636 [:db_insert_placeholder_4] => fr [:db_insert_placeholder_5] => 1 [:db_insert_placeholder_6] => 1 [:db_insert_placeholder_7] => 0 [:db_insert_placeholder_8] => 1483980082 [:db_insert_placeholder_9] => 0.5 [:db_insert_placeholder_10] => 0 [:db_insert_placeholder_11] => 0 [:db_insert_placeholder_12] => 0 ) in drupal_write_record() (regel 7376 van /var/www/***/includes/common.inc).

jovial_prash’s picture

#76 is working fine.
Please follow steps given below so as to create xmlsitemaps for multiple enabled languages separately.

1. Apply the patch.
2. To fix PDOException issue please run 'xmlsitemap_update_7204' hook_update.
2. enable 'xmlsitemap_i18n' module.
3. this will create lists of multiple sitemaps (depending on number of languages of the site) at XMLSITEMAP configuration page. If it fails to create on its own then you can add XMLSITEMAPS on your own in multiple enabled languages.
4. Edit required content 'include xml sitemap and save'.
5. refresh sitemap config page-----you will get those edited content (number) in links column.
6. sitemap links will go to 404 initially to fix that 'edit and save again those sitemaps'.
7. xml sitemaps should work now.

jovial_prash’s picture

Status: Needs work » Reviewed & tested by the community
ThuleNB’s picture

Are there any plans to committ this patch soon?

fmmribeiro’s picture

i used patch #90 but i found problem.
If I need a specific setting for a node, the setting doesn't get saved.

Neograph734’s picture

Status: Reviewed & tested by the community » Needs work

This should not have been marked as RTBC. From #93 we already knew there were still issues with the proposed patch in #90 and the newer patch fails testing.

Reverting to needs work. The patch is not yet complete.

dev.patrick’s picture

Tried #76 and this seems to resolve issue. Any plans to commit this patch?

cluke009’s picture

Tried the #93 patch, ran updates and it worked for me. This on a fairly complex production site and testing went smoothly. What exactly is holding up this patch?

alexei.orlov’s picture

#90 or #93 works fine for me, except for vocabularies with 'Localize' translation mode: links from these kind of vocabularies are no further generated. Not applying the patch changes to xmlsitemap_taxonomy.module make it works again. I guess that changes to this module should check the translation mode of the vocabularies...

bramvandenbulcke’s picture

I have been following this thread for about two years, waiting for a final patch. Also, it seems the Node Translation XML sitemap module (https://www.drupal.org/project/node_translation_sitemap) stopped being further developed because this functionality would be merged into the XML Sitemap module: https://www.drupal.org/node/2220641.

Patch #93 is working fine, one sitemap with almost all the pages and articles. Only issue I'm seeing at the moment is the frontpage, which has only been added in the main language. Normally, when I use XML sitemap I have to add View pages with the XML Sitemap custom submodule. Adding a View page with Entity Translation is not possible, because the node id has already been added in another language. These two are the same problem I suppose: the error shown on the XML Sitemap custom screen is "There is already an existing link in the sitemap with the path node/[nid]."

But anyway: thanks for all the effort put into the patches.

pavel.zheldak’s picture

Make add_support_for_entity_translation-1481798-90.patch compatible with patch indroduced in https://www.drupal.org/node/1670086 for rel="alternate" suport.

ciss’s picture

@pavel.zheldak Could you please provide an interdiff for your changes?

ciss’s picture

This is a reroll of #93, using a 3way merge against ffe35e1. I'm not sure what the point of #107 was (given that it was done for an older patch), so i hid the patch to minimize confusion.

Outstanding issues according to previous comments:

  • xmlsitemap_link_load from function xmlsitemap_node_create_link must take $language variable also, if we need to add entity translation support by correct way (#93)
  • vocabularies with 'Localize' translation mode: links from these kind of vocabularies are no further generated (#105)
  • frontpage has only been added in the main language (#106)

Setting to Needs Review to trigger tests.

Status: Needs review » Needs work

The last submitted patch, 109: add_support_for_entity_translation-1481798-109.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

attiks’s picture

Applied patch and enabled "xmlsitemap_i18n xmlsitemap_node xmlsitemap" works well, didn't test other sub modules

Nchase’s picture

#109 works for me but still doesn't solve the hreflang issue. Instead of heaving a sitemap for each language there should be one with hreflang, which is then in accordance with Google. There is effort put into enabling hreflang: https://www.drupal.org/project/xmlsitemap/issues/1670086

fprevos2’s picture

I'm having an issue with some of our sites. I'm not sure if it because we are using field translation or if we have some data issue but it does not seem to detect the proper language. When I have the option "Include links to untranslated entities to localized sitemaps" checked, the xmlsitemap tables contain content for 'und', 'en' and 'fr'. But the content that is mark has 'und' is duplicate from 'en' and 'fr' so it displays as duplicate on the xml sitemap. When the option "Include links to untranslated entities to localized sitemaps" checked, the xmlsitemap tables contain content for 'und' and 'fr' only. The content that is mark has 'und' is duplicate from the 'fr' and again display as duplicate content on the French xml sitemap. Since only the 'und' is displayed on the English xml sitemap, there is no duplicate there.

fprevos2’s picture

I wrote a patch to add another option in the admin menu to exclude the language neutral ('und') from being in the resulting xmlsitemap (when translation is enable for that entity). It seems to fix my issue with duplicate content when the default language is set to Language neutral but translation exist.

pifagor’s picture

Test fail

marco-s’s picture

Patch #114 works for me with the current dev version (dev-2.x e47457f).

But there's a small issue in the node update hook. It isn't possible to set a custom sitemap status (inclusion), because the link bundle status is always 1. I've updated the patch accordingly.

drupalfan2’s picture

Is this patch still necessary for latest Drupal 7 version? I am using #76.

mdixoncm’s picture

I've rolled the patch from 76 against the latest 2.4 release (the security release that was out last night). It has a minor change to how node updates are handled, in that it handles the processing on a queue rather than synchronously in the hook_node_update().

cmseasy’s picture

I am following this issue. What are the point(s) keeping the patches (latest #118) from status 'Needs review'?

othermachines’s picture

@mdixoncm - Which patch did you re-roll? Is that #109?

drupalfan2’s picture

Can not apply patch #76 on newest version 7.x-2.4 .

othermachines’s picture

@drupalfan2 - Read the latest comments and try the patch in #118. I believe it's a re-roll of #76 (although @mdixoncm should confirm).

mdixoncm’s picture

Sorry, yes I could have been clearer couldn’t I :)

It’s a re-roll of 76 - which we have been using on a few sites now for some time - had a mild panic this morning when I realised it didn’t apply to the latest release :)

othermachines’s picture

@mdixoncm - Well, I appreciate you doing that! Would you mind clarifying that in your comment (with the patch)? I'm sure there will be more folks coming here in search of your patch, and it might make it easier on them.

ciss’s picture

@mdixoncm Why did you reroll #76 instead of #109? Since #76 several issues have been mentioned that aren't covered by #76.

@fprevos2 and @marco-s: Can you please add interdiffs for your patches? In general, any patch that introduces functional changes should be accompanied by an interdiff.

@all If you reroll older patches just for the sake of having them still apply, please take care to hide them from the file list so that future work is not accidentally based on them.

mdixoncm’s picture

@ciss - because we have been using the patch in 76 for a long time, and I needed to get the security release out by 6am, so I just went with what we knew ... figured it might help someone out so posted here :)

othermachines’s picture

@mdixoncm Why did you reroll #76 instead of #109? Since #76 several issues have been mentioned that aren't covered by #76.

@ciss - If you asked for a show of hands, I'll bet a whole lot of the 93 followers of this issue are still using #76 since it stood for several months before any problems were detected. In the meantime, I think a re-roll of #76 is helpful while newer patches are awaiting review, but it should be clear that that is what it is.

@all If you reroll older patches just for the sake of having them still apply, please take care to hide them from the file list so that future work is not accidentally based on them.

Good call. And thanks for your efforts in trying to get things back on track. :)

zread’s picture

Here's a reroll of patch #116 against the latest DEV revision (commit 11b7c8ac475f9d1a186894956f01ed3e368d6707).

The patch includes a fix from https://www.drupal.org/project/xmlsitemap/issues/2987673, without which the sitemap links don't generate properly for languages.

orlando.thoeny’s picture

I rerolled the patch #128 against the latest dev (2.x-dev#99dc2e6) version, that includes a critical bugfix:

TypeError: Argument 1 passed to xmlsitemap_node_create_link() must be an instance of stdClass, boolean given
(https://www.drupal.org/project/xmlsitemap/issues/2986847)

othermachines’s picture

---------
Edit: Don't use this patch and interdiff! Refer to #131.
---------

Another re-roll of #116 against latest -dev. It removes a fix already committed in #2987673-5: Sitemap is not language aware (the fix mentioned in #128). The re-roll in #129 was missing a new file.

I am also adding an interdiff against #76 manually applied against -dev since it is still widely used.

I am going to *attempt* to outline the changes since then...

Changes since #76:

  1. Added fix suggested in #93
  2. Added new setting "Exclude links to language neutral entities to localized sitemaps" - Introduced in #114
  3. Fixed some spacing and text in docblocks.

There's probably more. Feel free to jump in.

Improvement needed:

xmlsitemap_node/xmlsitemap_node.module line 39

          $bundle_info = xmlsitemap_link_bundle_load('node', $node->type);
          $link_status = (int) $link['status'];

$bundle_info is superfluous here. $link_status used to point to (int) $bundle_info['status']; due to a change in #93 but was reverted (I think) in #116.

Next steps

1. Improve this patch
2. Review and fix "Outstanding issues" from #109
3. Test

Setting to Needs Review for testbot.

othermachines’s picture

Weird. I had previously uploaded the files before I'd discovered the missing xmlsitemap_i18n.install from #129. Even though I hit "remove" on those files , apparently d.o. thinks they are the same. Let's see if renaming the files helps.

The last submitted patch, 130: xmlsitemap-add_support_for_entity_translation-1481798-130.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 131: xmlsitemap-add_support_for_entity_translation-1481798-131.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phjou’s picture

For me, the last patch does not apply on the dev branch.

I was using the patch #90, but the security release has broken all the patches. The function xmlsitemap_node_cron has been totally changed.

phjou’s picture

Updated the patch for the latest commit on the branch dev: 046c34f1e192539c69becab69c5683f79928dc60

I can do an interdiff if needed but I don't know based on what because no patch work anymore

zread’s picture

I've tested patch #137 against 7.x-2.5 (aka 046c34f) and it seems to run fine on our end. It just needs to be adjusted to not fail the module's functional test.

hctom’s picture

I just also applied the patch from #137 and it applies cleanly... but I get errors during cron runs where the xmlsitemap gets updated;

WD xmlsitemap_node: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '725-node-0' for key 'PRIMARY': UPDATE {xmlsitemap} SET subtype=:db_update_placeholder_0, loc=:db_update_placeholder_1, language=:db_update_placeholder_2, access=:db_update_placeholder_3, status=:db_update_placeholder_4,                             [error]
status_override=:db_update_placeholder_5, lastmod=:db_update_placeholder_6, priority=:db_update_placeholder_7, priority_override=:db_update_placeholder_8, changefreq=:db_update_placeholder_9, changecount=:db_update_placeholder_10
WHERE  (type = :db_condition_placeholder_0) AND (id = :db_condition_placeholder_1) ; Array
(
    [:db_update_placeholder_0] => page
    [:db_update_placeholder_1] => node/725
    [:db_update_placeholder_2] => 0
    [:db_update_placeholder_3] => 1
    [:db_update_placeholder_4] => 1
    [:db_update_placeholder_5] => 0
    [:db_update_placeholder_6] => 1469181411
    [:db_update_placeholder_7] => 0.7
    [:db_update_placeholder_8] => 0
    [:db_update_placeholder_9] => 3769283
    [:db_update_placeholder_10] => 19
    [:db_condition_placeholder_0] => node
    [:db_condition_placeholder_1] => 725
)
 in drupal_write_record() (line 7387 of /docroot/includes/common.inc).

As you can see there is always 0 used for the langcode, which results those integrity constraint violations. I'll try to see if there is something I can do to fix this.

hctom’s picture

@phjou .. just one more question: Did you choose the patch form #90 for your reroll? Would be nice to have that information, to see what is really used now. Perhaps some maintainer should also take a look at this issue to clean up patches that were developed in different directions, which makes this issue quite unresolvable right now.

zread’s picture

@hctom Based on a diff comparison, #137 is a direct reroll of patch #131.

hctom’s picture

@zread thanx for the info. That's good, so we don't update outdated patches anymore ;)

I also found the problem with the patch that created the errors outlined in #139: The return value differed for xmlsitemap_get_entity_languages() in case the default language was returned.

Default language was returned this way:

array('langcode');

But available translations are returned this way:

array(
  'langcode' => 1,
);

So attached is a patch (based on patch from #137) that fixes this issue and returns the default language as a keyed array with status as value, too.

hctom’s picture

Status: Needs work » Needs review

So lets get this reviewed, but keep the "Next steps" section of #130 in mind for this.

zread’s picture

+1 for patch #142

ThuleNB’s picture

Patch 142 works for me too.

saurabh-chugh’s picture

patch #142 works +1

mistrytheory’s picture

Patch #142 isn't working for me. I'm seeing a significant drop in links when applying the patch locally, compared to what we have on our production site. I can't seem to see any immediate errors appearing either.

zread’s picture

@mistrytheory: Can you please provide more information? Like what version of xmlsitemap were you running before & with what patch? Have you ensured that all xmlsitemap-related Cron jobs have ran? (This is a fundamental difference with older versions of xmlsitemap, which could explain why you aren't immediately seeing as many links.)

hctom’s picture

@mistrytheory Can you please clarify/answer the questions asked by zread? As you are the only one so far who experiences problems with the patch, it would be nice to help us find potential problems.

By the way: when we applied the patch, we also had a drop in links locally. But after forcing a rebuild of all sitemap links via the admin UI and running the cron a lot of times with regenerating all cached sitemap files afterwards, we had exactly the same number of links as before.

mistrytheory’s picture

Apologies, I only got around to seeing the above comments now.

I tried this on 7.x-2.5 (and I believe for 4 as well).

@hctom we did something similar as well whilst testing; updating the cached files and rebuilding the sitemap, but no luck.

The only thing we hadn't run is the cron job, maybe that'll fix the issue? I'll endeavour to look into this, this week.

sabbaghian’s picture

Patch #142 works for me. Of course after running cron.

zread’s picture

Status: Needs review » Reviewed & tested by the community

Marking this as RTBC.

@mistrytheory: You absolutely have to run Cron. It's a major change introduced in xmlsitemap itself since version 2.4/2.5.

Ant.on’s picture

@zread: I updated the xmlsitemap module, implemented the #142 patch , ran cron, cleared my caches, and I still am not seeing the translated URLs in my sitemap. This is a broad question, but do you or anyone else have any ideas on what I could have missed? I double checked the diffs for my patch (Had to do it manually) and they are the same, so nothing in the patch was missed.

Any thoughts are super appreciated.

hctom’s picture

@Ant.on: Did you follow my comment #149? We really had to rebuild all links via UI, run cron several times and regenerate cached sitemap files and then everything was okay again.

bojan_dev’s picture

I was getting the following error on updb: Cannot add primary key to xmlsitemap : primary key already exists.
The language index should be dropped before it get's added as primary key, this has been covered now in the #155 patch.

Status: Reviewed & tested by the community » Needs work
bojan_dev’s picture

zread’s picture

@bojan_dev: Your patch removes xmlsitemap_i18n/xmlsitemap_i18n.install. Please fix this and ideally attach an interdiff between yours & patch #142.

bojan_dev’s picture

@zread: Thanks for your input, fixed the patch and added an interdiff.

hctom’s picture

@bojan_dev: I just looked into your new patch and I think there is something wrong with it. Just open up the patch from #142 and your patch and you will see, yours has a lot more changes in it, that are not in your interdiff. Can you please check and provide another patch? I hide your files in the meantime, so nobody uses them as there are definitely some problems with them. Thanx.

bojan_dev’s picture

@hctom: Indeed there was something weird going on, I have double checked it now, hopefully it's now oke :)

ThuleNB’s picture

Against which version of the module should I apply the patch?

iamfredrik’s picture

Patch #142 gets applied to the latest dev without errors, but it removed seven links. The sitemap had 128 links before and now only 121.

Patches #159 and #161 causes the following error:

Fatal error: Cannot redeclare xmlsitemap_update_7204() (previously declared in /sites/all/modules/contrib/xmlsitemap/xmlsitemap.install:574) in /sites/all/modules/contrib/xmlsitemap/xmlsitemap.install on line 585

bojan_dev’s picture

I have rerolled the patch to the latest dev, I guess the update_7204 has been added in the meanwhile, so I have renamed the update to 7205.

ThuleNB’s picture

I tried patch #142 with the latest dev version but my site breaks. Can somebody send me a patched version, please? I don't find the mistake.

Ant.on’s picture

@hctom,
Sorry for not replying sooner. The issue I was trying to implement this patch for isn't going to be used, however your suggestion worked!
I re-ran cron/cleared cache/rebuilt links enough times that it seemed to work!

Thanks!

therocket_gr’s picture

thanks for patch #164.. works fine

guardian87’s picture

Hi all,

Can someone tell me which patch works for the current 2.6 release of xmlsitemap?

So not the latest dev, but the current 2.6 version.

Many thanks

steva1982’s picture

Hi,
Any news?
Thank you.

Ste

litzastark’s picture

Confirming that patch #164 worked smoothly against version 2.6. Thank you!

PhilY’s picture

Also confirming patch #164 works using Drupal 7.64 and xmlsitemap 2.6
I had to disable, uninstall and reinstall all xmlsitemap modules to have the patch working (xmlsitemap, xmlsitemap_i18n and xmlsitemap_node)...
Thank you guys for you work.

sergeimalyshev’s picture

Using the last patch I can't update the xmlsitemap status of the taxonomy terms.
I've added small fix to patch #164.

sergeimalyshev’s picture

Status: Needs work » Needs review
ykyuen’s picture

#172 works. thanks

ciss’s picture

As a help for reviewers, here is the current patch history from #109 onward. Excerpts are taken from #172.

  • #114:
    Introduced the following code:
    +++ b/xmlsitemap.admin.inc
    @@ -406,6 +406,18 @@ function xmlsitemap_settings_form($form, &$form_state) {
    +  $form['advanced']['xmlsitemap_language_remove_neutral'] = array(
    +    '#type' => 'checkbox',
    +    '#title' => t('Exclude links to language neutral entities to localized sitemaps'),
    +    '#description' => t('If the option is enabled, it will not include links to language neutral to the localized sitemaps.'),
    +    '#default_value' => variable_get('xmlsitemap_language_remove_neutral', FALSE),
    +  );
    
    +++ b/xmlsitemap.module
    @@ -1747,3 +1770,48 @@ function xmlsitemap_link_enqueue($type, $ids) {
    +        if (variable_get('xmlsitemap_language_remove_neutral', FALSE) && !empty($languages[LANGUAGE_NONE])) {
    +          $languages[LANGUAGE_NONE] = 0;
    +        }
    
  • #116:
    Introduced the following code:
    +++ b/xmlsitemap_node/xmlsitemap_node.module
    @@ -34,7 +34,18 @@ function xmlsitemap_node_cron() {
    +          $link_status = (int) $link['status'];
    
  • #128:
    Possibly a faulty reroll, moves changes in xmlsitemap_node_node_update() to xmlsitemap_node_cron().
  • #129:
    Faulty reroll, drops entire xmlsitemap_i18n.install.
  • #131:
    Reroll, restores xmlsitemap_i18n.install.
  • #137:
    Reroll (to match refactoring), moves the following code:
    +++ b/xmlsitemap_node/xmlsitemap_node.module
    @@ -73,8 +84,7 @@ function xmlsitemap_node_xmlsitemap_process_node_links(array $nids) {
    -        $link = xmlsitemap_node_create_link($node);
    -        xmlsitemap_link_save($link, array($link['type'] => $node));
    +        xmlsitemap_node_node_update($node);
    
  • #142:
    Changes to the following code:
    +++ b/xmlsitemap.module
    @@ -1747,3 +1770,48 @@ function xmlsitemap_link_enqueue($type, $ids) {
    +  return array($default_language => 1);
    

    Interdiff:

    +++ b/xmlsitemap.module
    @@ -1800,3 +1800,3 @@
    -  return array($default_language);
    +  return array($default_language => 1);
    
  • #164:
    Adds the following code:
    +++ b/xmlsitemap.install
    @@ -576,6 +576,16 @@ function xmlsitemap_update_7204() {
    +  db_drop_unique_key($table, 'language');
    
  • #172:
    Applies change in #116, made for node, to taxonomy.
james.williams’s picture

Thanks for that great summary @ciss!

I'm using the patch from comment 172, which looks to be working reasonably so far.

However, if the 'Include links to untranslated entities to localized sitemaps' setting (xmlsitemap_language_fallback) is unticked, links to even published translations (including the original content) can end up removed from the sitemap. This is because calls to xmlsitemap_node_create_link(), which are supposed to get existing settings via xmlsitemap_link_load(), are not filtered by language and so just pick the first record from the {xmlsitemap} table. If that happens to be an unpublished translation, then you're out of luck - the record for your other translation will be updated with a zero status when cron comes along to update the access field for the record.

Presumably the same issue applies with xmlsitemap_taxonomy (etc?), though I'm not using that.

Given that we're adding language as a primary key to the xmlsitemap table, I think we need to start filtering by language everywhere possible too, to avoid this scenario. I'm out of time for today... hopefully I can submit a patch tomorrow!

james.williams’s picture

Here we are - this patch (built on top of patch 172, which is working for me otherwise) tries to ensure only the link record for the appropriate language is ever dealt with, rather than just loading up the first one. This allows the status to be different for each entity translation. Note that the rest of the settings (priority etc) are still kept the same across all languages. I haven't run the actual tests yet, let's see what happens...!

othermachines’s picture

Deleted unhelpful comment. Sorry for the noise. :-/

james.williams’s picture

@othermachines - that code is part of xmlsitemap_get_entity_languages(), which returns language keys, all mapped to 0 or 1. Perhaps the doxygen for that function should be improved as it's not accurate to what the function actually does.

othermachines’s picture

@james.williams - Yes, sorry about that. I was attempting to track down a particular issue I am having and didn't realize that function had been modified to that extent.

My issue is that, if a node is originally set to language neutral, then later translated, the language neutral entry isn't updated in the xmlsitemap table. The most critical side effect of this is, if the node is later unpublished, the link will still appear in the sitemap.

A simplified overview (active languages are 'en' and 'sv'):

1. Page is created as language neutral

xmlsitemap table:
en - access 1, status 0
sv - access 1, status 0
und - access 1, status 1

2. Page is switched to English (en)

xmlsitemap table:
en - access 1, status 1
sv - access 1, status 0
und - access 1, status 1

3. Page is unpublished.

xmlsitemap table:
en - access 0, status 1
sv - access 0, status 0
und - access 1, status 1

I think the issue might be that xmlsitemap_get_entity_languages() should always return LANGUAGE_NONE ('und') as part of the result set (and access/status set accordingly), not just when the entity is untranslated. Not sure, though.

I'd sure appreciate your thoughts!

We are using the latest -dev of this module + the latest patch in #177.
Language fallback is enabled.
The two settings provided by this patch are disabled.
We don't use i18n.

james.williams’s picture

Sure - so language neutral content should not be translated, by definition. If content is 'neutral' with respect to language, the whole point of that is that it should not be different between languages. I have seen corrupted content that has got into a bad state, with a 'neutral' translation alongside translations in 'actual' languages, but that should be totally unsupported. Drupal core and key multilingual modules ill also struggle if data ever gets into an incoherent state like that anyway.

So I don't think there's a problem there?

That said, it's often possible to change the language of content - so if it is originally created as language neutral, it could be changed to English/etc, and then truly translated into another language. So if you're testing, that might be a scenario to check (I haven't looked at the code recently). I'd hope we already cope with that, because the language neutral version should be removed at the point of first changing the content to English/etc.

othermachines’s picture

Thanks!

I am testing with fresh content. It occurs when a node is switched from 'language neutral' to English (or other language) before being translated. I should have been more explicit about that in my description, but I'm not sure it matters.

... because the language neutral version should be removed at the point of first changing the content to English/etc.

And therein lies my problem. It is not removed, nor updated. In my tests, when the content is first created as language neutral, xmlsitemap_get_entity_languages() (provided by this patch) returns all active languages, plus 'und', and these rows get added to the table. When the content is updated, it only returns the active languages. Therefore (presumably) only those rows in the table are updated while 'und' is left in its original state. This causes both duplicate links as well as links to unpublished content in the generated sitemap.

Anyway, I'll report back if I have any revelations. It's been awhile since I've had to look at this code, as well. I'm hoping someone more familiar with the patch might easily see the problem (and a fix). Thanks again -

joele75’s picture

I hope this is not a question that's been asked elsewhere and that it's relevant to this discussion.

I have a site (running on 8.8) that is largely based off a default English locale - in that most (95%) of pages were created in English and translated into German where needed. There are also some pages that were created only in German and not translated to English. At the moment, the sitemap entries for the English site are only including the nodes/pages that are in English which is fine I believe. The German sitemap.xml contains just the homepage. Is this expected behaviour? I understand that the hreflang tags in each English page should point the searchbots to their German equivalent so 95% of the German site is currently covered but the German only pages do not appear to be getting added. It might be that I've taken a non-standard approach to this but it would be good to hear that and to know whether this is something that can be solved with this package or whether that is unlikely.

proweb.ua’s picture

#172
when uninstalling module xmlsitemap_i18n this error

Error: Call to undefined function xmlsitemap_sitemap_get_context_hash() in xmlsitemap_i18n_uninstall() (line 57 of [error]
/var/www/vhosts/******www/sites/all/modules/contrib/xmlsitemap/xmlsitemap_i18n/xmlsitemap_i18n.install).

james.williams’s picture

aubergine2010’s picture

This patch is not working for me, language neutral content is not added to any sitemap.

james.williams’s picture

@aubergine2010 what do you have set for the 'Include links to untranslated entities to localized sitemaps' and 'Exclude links to language neutral entities to localized sitemaps' settings?
Are you in a place to be able to help diagnose why they're not appearing for you?

Language-neutral content is certainly handled by this patch; there are comments above discussing some specific scenarios around language neutral content, so it would be good to identify your scenario.

thomaswalther’s picture

So is this module ready for multilanguage? No fix for this 7 version? Is this error still in 8.x-1.2?