Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
15 Mar 2024 at 15:18 UTC
Updated:
4 Apr 2024 at 08:19 UTC
Jump to comment: Most recent
Problem/Motivation
If we want to do #3428372: [PP-1] Stop installing system module before everything else we need sessions to work without system module being installed first. We've got an established pattern for this already used for cache/batch/dblog etc. and SessionHandler can implement the same thing.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3428565
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
catchComment #4
catchComment #5
phenaproximaSeems relatively straightforward, but I have some complaints about the code. :)
Comment #6
catchAddressed all the feedback I think.
Comment #7
andypostLooks in combination with #2865991: provide an API to forcibly start a session there will be single place for sessions to start and create its storage
Comment #8
phenaproximaA 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...?
Comment #9
larowlanOne 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.
Comment #10
phenaproximaEnsuring @catch is credited for the patch, and I'm credited for my impeccable code review (hah!)
Comment #11
catchWe 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.
Comment #12
phenaproximaNo objections here.
Should we remove the
sessionstable fromsystem_schema()in this MR, per the change record?Comment #13
phenaproximaOpened #3431309: Services that lazily create database tables should catch SchemaObjectExistsException and SchemaObjectNotExistsException as needed, rather than Exception for #11.
Comment #14
alexpottDon'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.
Comment #15
catchDon't know how I missed the most important and most satisfying part of the change...
Comment #16
catchOK the answer to 'why doesn't this use
SchemaObjectDoesNotExistExceptionis 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\Throwablein the MR.@phenaproxima I had already opened #3431289: Update lazy database creation pattern for more specific exceptions etc., we should consolidate those two and not mention
SchemaObjectDoesNotExistException.Comment #17
phenaproximaHuh, too bad about SchemaObjectExistsException. The change to Throwable makes sense. RTBC assuming all tests pass.
Comment #18
catchI 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.
Comment #19
phenaproximaWell, that's a sadness.
Comment #20
catchTest 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 #3428372: [PP-1] Stop installing system module before everything else 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 #3431438: SessionManager should not directly query the sessions table.
Comment #21
phenaproximaNo objections.
Comment #22
alexpottThrowable 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 #2721271: Simplify exception handling for lazy-load pattern and try to standardise this in some way.
Left some other feedback on the MR.
Comment #23
catchYeah 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.
Comment #24
catchComment #25
catchOnly applying @alexpott's suggestions (either literal gitlab ones or review points), so moving back to RTBC.
Comment #26
alexpottCommitted and pushed da2e2ce2ae to 11.x and 9feabd3759 to 10.3.x. Thanks!