Problem/Motivation

We create the url_alias table on the installation of the system module.
Let's make that lazy like for example the cache system.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#24 url_alias_table_is_only-2664466-24.patch31.48 KBkostyashupenko
#21 2664466-21.patch31.49 KBdawehner
#13 2664466-13.patch32.72 KBalexpott
#13 11-13-interdiff.txt1.23 KBalexpott
#11 2664466-11.patch31.49 KBalexpott
#11 9-11-interdiff.txt3.74 KBalexpott
#9 2664466-8.patch35.23 KBalexpott
#8 6-8-interdiff.txt7.96 KBalexpott
#6 2664466-6.patch26.5 KBdawehner
#3 2664466-3.patch26.44 KBdawehner
#2 2664466-2.patch42.5 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Active » Needs review
FileSize
2664466-2.patch42.5 KB

Let's see

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2664466-3.patch26.44 KB

That included both routing and url_alias.

Status: Needs review » Needs work

The last submitted patch, 3: 2664466-3.patch, failed testing.

The last submitted patch, 2: 2664466-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2664466-6.patch26.5 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 6: 2664466-6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
6-8-interdiff.txt7.96 KB

Fixing the fails...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2664466-8.patch35.23 KB

Here's the patch

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Needs work

Patch needs a rebase, includes unwanted changes of filestorage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
9-11-interdiff.txt3.74 KB
2664466-11.patch31.49 KB

Unfortunately I had applied #2620576: fnmatch() is not available on all environments (i.e QNAP QTS) :(

Here's the correct patch - the interdiff to #6 in #8 was actually correct.

Looks like we have some postgres issues...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Oh yay - postgres has an opinion about transactions and this auto-create table stuff... https://stackoverflow.com/questions/10399727/psqlexception-current-trans...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
11-13-interdiff.txt1.23 KB
2664466-13.patch32.72 KB

Phew all of the postgres query types create a savepoint - so should insert... which fixes this.

We need this anyway because if you write to an uncreated cache bin during a node save postgres would break and that's wrong.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

#2347867: Race conditions with lock/cache using non-database storage - add a non-transactional database connection would also help with this, but #13 looks good.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented 
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -99,12 +99,21 @@ public function execute() {
+    $this->connection->addSavepoint();

IMHO we should document this here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I think we should split the postgres fix out into its own issue so we can add tests - and hey presto we have an issue already... #2487269: Postgres insert queries that fail in a transaction break the entire transaction

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Related issues: +#2487269: Postgres insert queries that fail in a transaction break the entire transaction
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Postponed

Postponing on #2487269: Postgres insert queries that fail in a transaction break the entire transaction.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Postponed » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2664466-13.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2664466-21.patch31.49 KB

Rebased

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs review » Reviewed & tested by the community

And again.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Sorry needs a re-roll.

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Needs work » Needs review
FileSize
url_alias_table_is_only-2664466-24.patch31.48 KB

Re-rolled patch from #21 with auto merge

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

rerolled

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

  • catch committed 9040dc8 on 8.1.x 
    Issue #2664466 by dawehner, alexpott, kostyashupenko: url_alias table is...
edurenye’s picture

edurenye CreditAttribution: edurenye at MD Systems GmbH commented

I opened this follow up #2670360: Add BC layer in installSchema to support url_alias
To add the BC layer to fix the contrib test fails in the testbot.

Status: Fixed » Closed (fixed)

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

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented

Honestly I think this needs to be rolled back. We need to rely on being able to query the table directly in Pathauto, because the service does not provide some of the actual querying we need.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented

Why couldn't we have just moved everything (schema, service, etc) to the path module?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented

url_alias table is only used by a core service

And there is the fallacy. The needs of contrib completely ignored. :(

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Related issues: +#2704821: url_alias table should be created by system install