Closed (fixed)
Project:
Simple XML sitemap
Version:
8.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Anonymous (not verified)
Created:
9 Aug 2016 at 21:33 UTC
Updated:
22 Jul 2017 at 12:42 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
Anonymous (not verified) commentedRyan Weal created an issue. See original summary.
Comment #2
Anonymous (not verified) commentedPatch for the 8.x-2.x development branch.
Comment #3
Anonymous (not verified) commentedA patch for the current stable branch, for those who are having trouble "in production"... works on 8.x-2.5.
Comment #5
Anonymous (not verified) commentedRe-uploading the patch against the development branch. Testbot: can I get a little love?
Comment #7
Anonymous (not verified) commentedComment #9
Anonymous (not verified) commentedComment #10
Anonymous (not verified) commentedFinally an updated patch for the stable version...
Comment #11
Anonymous (not verified) commentedHide patches that failed.
Comment #12
gbyteHi 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?
Comment #13
Anonymous (not verified) commentedI 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.
Comment #14
adamwhite commentedI 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.
Comment #15
gbyteThanks 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:
instead of
I feel like this next line should have its own method (if we decide to use str_replace), but this is nitpicking:
Thanks for your time!
Comment #16
gbytePlease do not forget to pull the new dev version first, as there has been lots of code refactoring to improve testability.
Comment #18
gbyteComment #19
hitfactory commentedAttached is a reroll of patch in #10 I am using on Platform.sh against 8.x-2.x-dev.
Comment #21
gbyte@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?