To improve sitemap generation performance, xmlsitemap_get_path_alias() skips calling url(). This has a side effect of skipping any hook_url_outbound_alter() implementations, notably in modules like subpathauto.
The attached patch fixes this, allowing for URLs like node/1234/photos to be properly exported as clean URLs in site maps. We currently have a custom module creating links for these paths, but I expect a similar situation could be recreated by installing subpathauto and adding a custom link through the UI for node/1/edit.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | xmlsitemap-2378467-20.patch | 1.67 KB | pifagor |
| #13 | 2378467.12-url-outbound-alter.patch | 3.52 KB | deviantintegral |
Comments
Comment #1
deviantintegral commentedComment #5
robcolburn commented@deviantintegral,
SimpleTest failed, why?!?
Comment #6
dave reidThis was actually done for performance reasons since calling the full URL aliasing could be very expensive for a large sitemap, so we should make this optional?
Comment #7
deviantintegral commentedI actually tried to post that we should retest this as it looked like testbot failed, but d.o WSOD'ed on me :(
Here's a new patch fixing a potential notice.
Comment #8
deviantintegral commentedAlso - aliasing isn't handled in hook_url_outbound_alter(), but directly in url() which calls drupal_get_path_alias(). So, I think this shouldn't cause any performance issues for sites with large numbers of sitemapped-but-unaliased URLs.
Comment #9
robcolburn commentedRe-ping Testbot
Comment #12
robcolburn commented@DaveReid,
Green lights from Testbot, bumping up for another look after @deviantintegral's last comment.
Comment #13
deviantintegral commentedFixes more notices due to me overwriting $options instead of just a single key.
Comment #14
deviantintegral commentedComment #15
robcolburn commentedComment #16
dave reidAfter review this seems like a good change and thanks for all the good feedback and testing.
Leaving as needs port to D8, not sure how this code changes in the ported code.
Comment #18
dave reidJust an FYI this missed that $options['external'] needs to always be defined. Fixed in follow-up commit.
Comment #20
pifagor commentedI added the patch but was not able to test it
Comment #21
pifagor commentedneed to reroll
Comment #22
neclimdulFWI, everything seems to go through \Drupa\Core\Url and core's url generator in d8 and doesn't seem to use this.
Best I can tell the code in #20 isn't used and the only reference was removed here:
https://git.drupalcode.org/project/xmlsitemap/-/commit/82d4fecb20de58872...
So it can probably be removed and this isn't needed in 8.x at least for now.
Comment #23
dave reidYeah this isn't necessary anymore in D8 since we're using the Url class, and it handles all that for us. Marking as fixed in D7.