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.

Comments

deviantintegral’s picture

StatusFileSize
new1.45 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2378467.1-url-outbound-alter.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2378467.1-url-outbound-alter.patch, failed testing.

robcolburn’s picture

@deviantintegral,

SimpleTest failed, why?!?

Only local images are allowed.

dave reid’s picture

This 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?

deviantintegral’s picture

StatusFileSize
new2.57 KB

I 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.

deviantintegral’s picture

Also - 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.

robcolburn’s picture

Status: Needs work » Needs review

Re-ping Testbot

Status: Needs review » Needs work

The last submitted patch, 7: 2378467.7-url-outbound-alter.patch, failed testing.

Status: Needs work » Needs review
robcolburn’s picture

Status: Needs review » Reviewed & tested by the community

@DaveReid,
Green lights from Testbot, bumping up for another look after @deviantintegral's last comment.

deviantintegral’s picture

StatusFileSize
new3.52 KB

Fixes more notices due to me overwriting $options instead of just a single key.

deviantintegral’s picture

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

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

Version: 7.x-2.x-dev » 8.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

After 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.

dave reid’s picture

Just an FYI this missed that $options['external'] needs to always be defined. Fixed in follow-up commit.

  • Dave Reid committed 56e0c2a on 7.x-2.x
    Issue #2378467: Fixed undefined $options['external'] when calling...
pifagor’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new1.67 KB

I added the patch but was not able to test it

pifagor’s picture

Status: Needs review » Needs work

need to reroll

neclimdul’s picture

FWI, 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.

dave reid’s picture

Version: 8.x-1.x-dev » 7.x-2.x-dev
Status: Needs work » Fixed

Yeah 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.

Status: Fixed » Closed (fixed)

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