Follow-up to #2664466: url_alias table is only used by a core service but it depends on system install.

Problem/Motivation

We made a mistake making url_alias auto-creatable. This means that until a url_alias is created we throw and exception on many/every requests - see #2696813: url_alias table missing?

Proposed resolution

Re-add the table to system_schema. Reverting #2664466: url_alias table is only used by a core service but it depends on system install is painful and unnecessarily complex and risky.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
Status: Closed (fixed) » Active
Related issues: +#2696813: url_alias table missing?
alexpott’s picture

Category: Task » Bug report
Priority: Normal » Major
alexpott’s picture

Status: Active » Needs review
StatusFileSize
new786 bytes

How to test...

alexpott’s picture

Issue tags: +rc target triage
StatusFileSize
new982 bytes
new1.73 KB

I think given the situation we should consider committing this to 8.1.x-rc.

The test only patch is the interdiff.

The last submitted patch, 5: 2704821-5.test-only.patch, failed testing.

alexpott’s picture

Priority: Major » Critical

So yes this is going to use exceptions to communicate on every request because PathProcessorAlias is going to process every inbound path. Discussed this with @xjm and we agreed that fixing this is critical.

alexpott’s picture

However, I'm not sure that the cost of exceptions is that much given this http://zurb.com/forrst/posts/Speed_performance_of_Trying_and_catching_Ex...

berdir’s picture

Looks good to me.

I haven't tested how slow it actually it, but to be fair, it's not just throwing of a plain exception. It's a pdo exception, that..

1. We catch in \Drupal\Core\Database\Connection::query
2. parse query string and error code in \Drupal\Core\Database\Connection::handleQueryException, re-throwing it
3. catch again in \Drupal\Core\Path\AliasStorage::load
4. do a table exists query in \Drupal\Core\Path\AliasStorage::catchException(), by querying information_schema.tables on mysql.

That can't be very fast :)

catch’s picture

The cache backends just eat exceptions on get.

#9-#4 looks like the problematic bit here - we either need to create the table, or not care that it doesn't exist.

To me looks like three options:

1. Stop calling $this->catchExeption() on read.
2. Create the table on read if it's missing
3. the patch here.

To keep things self-contained, I'd prefer #1 or #2 if we can, but I can also see doing #3 then revisiting this in a major for 8.2.x

berdir’s picture

The reason we check that is to know if it's a real error or not.

What I discussed with @alexpott in IRC is that we could introduce a special exception class that we'd identifiy based on the error code, we already have one example for that.

catch’s picture

There's no standard error code for missing tables, that's been discussed elsewhere and is the reason the cache backends just eat exceptions.

What about creating the table if it's missing on read?

catch’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -rc target triage +rc target

Let's do all that in a follow-up, opened one at #2706001: Remove url_alias table from system_install() again.

Definitely an rc target for me - this is a fatal introduced by changes in 8.1.x and the fix here is straightforward.

  • catch committed dcf8bb0 on 8.2.x
    Issue #2704821 by alexpott: url_alias table should be created by system...

  • catch committed 78ba568 on 8.1.x
    Issue #2704821 by alexpott: url_alias table should be created by system...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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