When multiple sitemaps of type "Default hreflang" are defined, only one can be overwritten

Created on 6 November 2023, 8 months ago
Updated 23 November 2023, 7 months ago

Problem/Motivation

When a site defines multiple sitemaps of type Default hreflang, only one of them can be overwritten in the node edit form.

Steps to reproduce

  • Install simple sitemap module
  • Add a second sitemap of type Default hreflang
  • Enable both sitemaps for a content type
  • Create or edit a node of that content type and change for both sitemaps a value, e.g. priority
  • Save node

The expected result is, that both sitemaps get an override for the node.

What actually happens is, that only one sitemap gets the override. The other sitemaps still shows the default values for that content type.

This does not happen with module versions below 4.1.7

I think the change, that introduced the bug was Show only relevant sitemap variants in UI Fixed

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Fixed

Version

4.0

Component

Code

Created by

🇩🇪Germany daniel.bosen

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @daniel.bosen
  • Status changed to Needs review 8 months ago
  • Merge request !83Iterate over all sitemaps → (Open) created by daniel.bosen
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.1 & MySQL 8
    last update 8 months ago
    32 pass
  • 🇩🇪Germany daniel.bosen

    This fixes the problem for me.

  • Assigned to WalkingDexter
  • Assigned to gbyte
  • Status changed to Needs work 8 months ago
  • 🇷🇺Russia WalkingDexter

    @daniel.bosen Thank you for reporting! MR has changes in the wrong place, but the main idea is correct. I fixed the problem in the above commit.

    @gbyte This bug is a symptom of a bigger problem with SitemapGetterTrait. The list of set sitemaps is statically cached in the $sitemaps property. Sometimes this leads to non-obvious results if some code previously called the setSitemaps() method and specified a parameter other than NULL. In other words, in some cases the getSitemaps() method is expected to return a list of all compatible sitemaps, but instead it returns only those that were previously set.

  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.1 & MySQL 8
    last update 8 months ago
    32 pass
  • 🇮🇳India Rajeshreeputra Pune

    Patch was not applying hence rebased but I can see the fix is added in dev, will test without this patch. Thank you @WalkingDexter!!

  • First commit to issue fork.
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.1 & MySQL 8
    last update 7 months ago
    5 pass, 2 fail
  • 🇺🇦Ukraine HitchShock Ukraine

    The bug really exists.
    The functions setEntityInstanceSettings() and getEntityInstanceSettings() don't support multiple variants. And we also have a @todo in the code to fix it.
    See https://git.drupalcode.org/project/simple_sitemap/-/blob/4.x/src/Manager...

    I provided a quick solution for that.
    Updated MR to fix it. Also added a patch to simplify the application of the solution.

    But.. the tests must be updated too.

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.1 & MySQL 8
    last update 7 months ago
    32 pass
  • 🇺🇦Ukraine HitchShock Ukraine

    Readded the patch without tests for v4.1.7

  • Status changed to Needs review 7 months ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.1 & MySQL 8
    last update 7 months ago
    Patch Failed to Apply
  • Issue was unassigned.
  • Status changed to Fixed 7 months ago
  • 🇩🇪Germany gbyte Berlin

    @HitchShock

    I have not had time to go through your code thoroughly, but please note the following:

    When I informed you in 🐛 Entity Instance Settings - support multiple sitemap types Closed: outdated that the bug you are getting is probably rooted in a regression we've introduced in the last version, I meant it has already been fixed in this here issue in the dev version. The code todo you are referring to is about the setEntityInstanceSettings() method not handling multiple variants as other methods do, but that's just an implementation detail that has never stopped the module from working as intended. We've been working around this limitation by calling the method for each set variant instead of calling the method once.

    I have tested dev and it seems to be working well without your patch. As I mentioned in the issue linked above, you may need to reapply all settings. What I meant by this is, you may need to disable sitemap support for the entity bundle in question and then set any overrides anew.

    @WalkingDexter

    @gbyte This bug is a symptom of a bigger problem with SitemapGetterTrait. The list of set sitemaps is statically cached in the $sitemaps property. Sometimes this leads to non-obvious results if some code previously called the setSitemaps() method and specified a parameter other than NULL. In other words, in some cases the getSitemaps() method is expected to return a list of all compatible sitemaps, but instead it returns only those that were previously set.

    The getting and setting of sitemaps uses a clever mechanism that has a lot of advantages, but also this quite serious disadvantage. I don't intend to change the mechanism in a breaking manner, but I suggest we document this anti-pattern. How about adding a warning to the docblocks in question?

    I am setting this issue to 'fixed' - if any of you guys encounter the above problem, let me know.

  • 🇺🇦Ukraine HitchShock Ukraine

    Hi @gbyte
    Retested the issue with dev module version on my project more carefully and you are right. Seems like it was fixed by the last commit on the dev branch.
    So, yes, the issue is fixed. Made a patch for the last commit to apply it on the 4.1.7 version

  • 🇺🇦Ukraine HitchShock Ukraine

    Sorry, the wrong file was uploaded

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

Production build 0.69.0 2024