I have run into some trouble with this module when I run it on a system where drush/cron run under a different $base_url.

This value can no longer be overridden in settings.php so I am having to patch the module to allow for an override.

It is particularly problematic because if you submit your sitemap.xml to Google and the base path is wrong, you will be removed from the search index.

Patch coming...

Comments

Anonymous’s picture

Ryan Weal created an issue. See original summary.

Anonymous’s picture

Patch for the 8.x-2.x development branch.

Anonymous’s picture

Status: Active » Needs review
StatusFileSize
new3.7 KB

A patch for the current stable branch, for those who are having trouble "in production"... works on 8.x-2.5.

Status: Needs review » Needs work
Anonymous’s picture

Status: Needs work » Needs review
StatusFileSize
new3.15 KB

Re-uploading the patch against the development branch. Testbot: can I get a little love?

Status: Needs review » Needs work

The last submitted patch, 5: 2781627-not-possible-to-override-baseurl.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
StatusFileSize
new3.49 KB

Status: Needs review » Needs work

The last submitted patch, 7: 2781627-not-possible-to-override-baseurl-7.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
StatusFileSize
new3.97 KB
Anonymous’s picture

Finally an updated patch for the stable version...

Anonymous’s picture

Hide patches that failed.

gbyte’s picture

Status: Needs review » Postponed (maintainer needs more info)

Hi Ryan, I haven't had the chance to look at your patch yet, but it appears you are experiencing this issue: #2727863: Multiple domain names
Can you please try passing the uri parameter to cron and confirm it sorts out the problem?

Anonymous’s picture

Status: Postponed (maintainer needs more info) » Needs review

I can't pass any cron details on Pantheon. I would assume it is probably the same situation on Acquia Cloud and Platform.sh (though I don't know much about Platform.sh).

This is the same approach that is taken by xmlsitemap for this issue.

adamwhite’s picture

I was running into the same issue with our Pantheon hosted sites using this module. The patch in #10 is working well for us. We've got a mix of both nodes and taxonomy terms set to show up in the sitemap and both are now being added with our preferred base URL.

gbyte’s picture

Thanks for the patch Ryan, it appears there is a wider interest to implement this, so I will look into this next.

My review of the patch (without having applied it yet):
Why are #9 and #10 so much different and which one is right for dev? Please don't post patches against the stable version. Also it seems strange to use str_replace to replace the string coming from the URL object and I feel there must be a cleaner way. The URL class doesn't seem to have an option to alter the URLs base path. Anybody has an idea how to cleanly implement this?

There is an API within simple_sitemap for getting the module's settings, use the simple_sitemap.generator service's getSetting() method instead:

$base_url = \Drupal::service('simple_sitemap.settings')->getSetting('base_url')

instead of

$config = \Drupal::config('simple_sitemap.settings')->get('settings');
$base_url = $config['base_url'];

I feel like this next line should have its own method (if we decide to use str_replace), but this is nitpicking:

$urls[$language->getId()] = str_replace($GLOBALS['base_url'], $batch_info['base_url'], $url_object->toString());

Thanks for your time!

gbyte’s picture

Status: Needs review » Needs work

Please do not forget to pull the new dev version first, as there has been lots of code refactoring to improve testability.

The last submitted patch, 9: 2781627-not-possible-to-override-baseurl-9.patch, failed testing.

gbyte’s picture

Category: Bug report » Feature request
hitfactory’s picture

Status: Needs work » Needs review
StatusFileSize
new5.03 KB

Attached is a reroll of patch in #10 I am using on Platform.sh against 8.x-2.x-dev.

  • gbyte.co committed 4136ffa on 8.x-2.x
    Issue #2781627 by Ryan Weal, hitfactory: Not possible to override...
gbyte’s picture

Status: Needs review » Fixed

@hitfactory Thanks for rerolling, however some of my critique on the patch from #15 still remains.
I've changed the logic to the point I'm happier with the code and commited to dev. The logic could still be improved though. I've also added a form validator and a couple of tests. Can you guys test the functionality for me?

Status: Fixed » Closed (fixed)

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