When Lingotek module downloads translated URL aliases it checks them through \Drupal::pathValidator()->isValid(). The pathValidator service checks that the path is actually routed and accessible to the current user, thus it will always return false for new aliases.

If I understand the motivation behind the original issue and patch at #2844532: When URLs are translated with Lingotek, we must validate them before storing, the alias portions should probably just be using UrlHelper::isValid() to check syntax.

CommentFileSizeAuthor
#6 2928420-alias-not-stored-6.patch7.35 KBpenyaskito
#6 2928420-alias-not-stored-6.only-tests.patch5.36 KBpenyaskito
#6 2928420-alias-not-stored.interdiff.4-6.txt5.38 KBpenyaskito
#4 2928420-alias-not-stored-4.patch2.74 KBpenyaskito
#4 2928420-alias-not-stored-4.only-tests.patch1.24 KBpenyaskito
#2 2928420-alias-is-valid.patch1.13 KBkrlucas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

krlucas created an issue. See original summary.

krlucas’s picture

krlucas CreditAttribution: krlucas commented
Status: Active » Needs work
FileSize
2928420-alias-is-valid.patch1.13 KB

Attached is a quick and dirty patch. No tests yet so leaving as needs work.

charginghawk’s picture

charginghawk CreditAttribution: charginghawk as a volunteer commented
Status: Needs work » Needs review

Let's see what happens with the tests.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
FileSize
2928420-alias-not-stored-4.only-tests.patch1.24 KB
2928420-alias-not-stored-4.patch2.74 KB

Somehow I missed this issue :-/.

path.module already creates the alias from the alias property, so even with that patch we would be duplicating aliases, and the first one will be the original one. Setting the property and letting the path module do its job works for me.

See attached patch with tests. No interdiff but it's easy to spot the difference.

Status: Needs review » Needs work

The last submitted patch, 4: 2928420-alias-not-stored-4.patch, failed testing. View results

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek commented
FileSize
2928420-alias-not-stored.interdiff.4-6.txt5.38 KB
2928420-alias-not-stored-6.only-tests.patch5.36 KB
2928420-alias-not-stored-6.patch7.35 KB

Fixed the pathauto tests. Actually changed behavior, we don't want pathauto to override anything if there was a valid translation.

penyaskito credited Phil_b.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek for Pegasystems commented
Status: Needs work » Needs review

Setting to needs review for testbot.

Giving attribution to @Phil_b who also reported this issue and @charginghawk.

The last submitted patch, 6: 2928420-alias-not-stored-6.only-tests.patch, failed testing. View results

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek for Pegasystems commented
Status: Needs review » Reviewed & tested by the community

Tests passed and failed as expected.

  • penyaskito committed ab68126 on 8.x-2.x authored by krlucas 
    Issue #2928420 by penyaskito, krlucas, charginghawk, Phil_b: Downloaded...
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito at Lingotek for Pegasystems commented
Status: Reviewed & tested by the community » Fixed

Committed ab68126 and pushed to 8.x-2.x. Thanks!

charginghawk’s picture

charginghawk CreditAttribution: charginghawk as a volunteer commented

Thanks!

Status: Fixed » Closed (fixed)

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