Implement lazy database creation for sessions

Seems relatively straightforward, but I have some complaints about the code. :)

Addressed all the feedback I think.

Looks in combination with ✨ provide an API to forcibly start a session Needs work there will be single place for sessions to start and create its storage

A few more small points/questions, but otherwise this is probably ready. Since this is an internal refactoring and tests are passing, I don't think it needs explicit test coverage. But it might need a change record...?

One fun thing with these auto created tables is restoring a database when the site is under load.

Found that out the hard way when the restore died 80% through because the key value table had auto created itself. Solution was to shutdown the nginx server as well.

Ensuring @catch is credited for the patch, and I'm credited for my impeccable code review (hah!)

We should probably open a follow-up to bring some of the other subsystems that implement this up-to-date with SchemaObectNotExistsException etc., I think they were added after we started doing this.

No objections here.

Should we remove the sessions table from system_schema() in this MR, per the change record?

Opened 📌 Services that lazily create database tables should catch SchemaObjectExistsException and SchemaObjectNotExistsException as needed, rather than Exception Active for #11.

Don't we need to remove sessions from system_schema - otherwise we have to maintain the schema in two locations which seems wrong. Plus we're not really using the changes here if we're still creating the table during system module install.

Don't know how I missed the most important and most satisfying part of the change...

OK the answer to 'why doesn't this use SchemaObjectDoesNotExistException is that that exception is only thrown when you try to change a schema object that doesn't exist (like altering a table or something), not when just querying from a table that doesn't exists, so went back to \Throwable in the MR.

@phenaproxima I had already opened 📌 Update lazy database creation pattern for more specific exceptions etc. Active , we should consolidate those two and not mention SchemaObjectDoesNotExistException.

Huh, too bad about SchemaObjectExistsException. The change to Throwable makes sense. RTBC assuming all tests pass.

I appreciate the enthusiasm but all tests very much do not pass yet - removing the entry from system_schema() is finding bugs, thought this too easy when I pushed the initial changes.

Well, that's a sadness.

Test failures resolved I think:

1. The installer redirect trait hard-codes the {sessions} table, changed this to {sequences}. This will have to be tackled in 📌 Stop installing system module before everything else Needs work so doesn't need its own follow-up.

2. SessionManager hard-codes a database query to the {sessions} table. Added a further try/catch there and opened 📌 SessionManager should not directly query the sessions table Active .

No objections.

Throwable is way more generic than the other instances of this in core... not sure if that matters but it feels odd. They all go for \Exception which is still super generic but less :). I think we should try to work on 📌 Simplify exception handling for lazy-load pattern Needs work and try to standardise this in some way.

Left some other feedback on the MR.

Yeah it didn't seem to hurt catching throwable in these cases because there's not really a lot else that can go wrong in the code path anyway, but also makes it a bit pointless - switched back to Exception everywhere and also applied the two suggestions.

Replied in the MR but we already have a follow-up which will handle the installer check it's just got slightly wider scope than just that check, but IMO between that issue and the eventual Drupal 12 issue to remove the sequences table it won't get forgotten.

Only applying @alexpott's suggestions (either literal gitlab ones or review points), so moving back to RTBC.

Committed and pushed da2e2ce2ae to 11.x and 9feabd3759 to 10.3.x. Thanks!

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