URL alias load is inconsistent if there are more then one alias

The problem is that path_load() doesn't do ORDER BY at all.

However, is there a reliable way to sort aliases by age, looking at the nasty code in drupal_lookup_path() it seems to be highly conditional.

This is also noticeable when using http://drupal.org/project/globalredirect and visiting the /node/xxxx path. Redirects to the wrong alias.

Why do we sort it in ascending order?

I think a sort order just needs to be decided on and made consistent.
Otherwise this bug will end up slipping in to Drupal 9.x !

See also #1934086: path_load and drupal_lookup_path are inconsistent when choosing the 'current' path of an entity.

ianmthomasuk's patch sorts the opposite way as agentrickard's in #1. I agree with ianmthomasuk and his rationale that path_load should order the same way as drupal_lookup_path. The alias shown to the editor when editing a node should be the same one used by the path system as the main alias, and they should both be the newest alias.

This is a pretty bad bug in D7. You can edit a node and change the alias and have that have no effect.

Are there any reasons this should be set to ASC rather than DESC? I've not tested agentrickard's patch, but I wouldn't have thought it actually fixes the problem as described in the issue summary since 'alias2' will have a higher pid than alias1 and therefore not be returned by path_load.

I'd say these bugs are duplicates of each other. If we can agree that the order should be DESC then I'd say this is RTBC for Drupal 8.

I'm a bit more wary on Drupal 7 though - is there any possibility that sites are relying on the current, buggy behaviour?

DESC is better, or else the contributor will never understand the logic. As #2 states, global redirect will always redirect to the latest.

I've also RTBC the other issue (Drupal 7)

The query should probably also put a range(0, 1) for optimisation because as it is written, it SELECT every row that matches the conditions and returns the first only, but the SQL engine will do and buffer/cache the query that SELECTs everything. LIMIT 1 OFSSET 0, will optimise it, even if it is insignificant, it's still better. This patch might be the best chance to apply it without having to open another issue and wait 6 month for a maintainer to commit it :) Agree DESC is the way to sort.

Re-rolled with DESC ordering and range limit.

I think this is RTBC.

It is.

Indeed.

I think this should be testable.

Tests attached.

Looks OK.

Committed 0e17d97 and pushed to 8.0.x. Thanks!

D7 patch from other issue (the same, but without the change to the test, I've not checked to see if there's a test needing changing in D7).

Dreditor suggested commit message (with added names from #24) was "Issue #1934086 by SebCorbin, ianthomas_uk, pounard, Dave Reid, agentrickard: Fixed path_load and drupal_lookup_path are inconsistent when choosing the 'current' path of an entity."

I strongly suggest to add Dave Reid, agentrickard and myself to commit credits for the sake of fame. So many people in a 2 line change :)

The patch in #23 is the same as was already posted in #15. The one in #23 is named after the other issue, which was closed as a duplicate.

Is this ever going to drop in to Drupal 7 Core before it's too late?

I raised a concern above that some sites might be relying on the current buggy behaviour. Having considered it again I don't think we need to worry about that. The current behaviour has no defined priority for path aliases, they are chosen based on the order the database happens to return them. Routine database maintenance or even other queries that have run recently could affect the order that a database returns rows, leading to a intermittent, difficult to diagnose bug. If this change does cause problems for anyone then at least they will be easy to reproduce and easy to fix by setting the correct alias through the UI.

This is already fixed in D8 and the change already had an RTBC in the duplicate issue #1934086. It needs someone to RTBC this for D7. I can't because I'm the patch author.

Re-uploading patch since the test infrastructure has changed since I uploaded it 3 years ago!

Will someone let me know, how there may be multiple alias exist for same node? As older alias gets override by new one after edit node. If there is such condition that a single node can have multiple alias, then please update.

Thanks:

@Parahram
I think there's a setting within pathauto module to just create a new alias instead of deleting the old one (See attached image).

Marking #34 as RTBC. We've been using this in production for some time. The codesniffer failures seem to be an code-style issues unrelated to this change.

Hello,

When could we have this patch #34 committed in D7?
We meet an issue related to presence of multiple alias for the same nodes and the approach proposed by the patch fixed it.

I am using the D7 patch from #34 on some projects without any problems.

Reuploading this patch with an added test to show the failure of path_load(). It should fail at least in some DB engines (for example PostgreSQL). Patch itself is unchanged.

This looks good and it's great to have the test.

#33 makes sense re. the currently undefined db behaviour in the absence of an ORDER BY but still worth a brief Change Record to warn people that the behaviour may change (but will now be consistent).

Thanks everyone who contributed! Commited and pushed to 7.x.

Drafted a change record here: https://www.drupal.org/node/3307766