Problem/Motivation
The primary goal of this issue is to get from:
drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
$kernel = new DrupalKernel(...);
to
drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);
$kernel = new DrupalKernel(...);
in index.php.
A secondary goal is to remove the "bootstrap container", or at least reduce it to contain only services that are necessary for the kernel to build the full container. The hope is that there won't be any such services.
Whatever approach we take, we need to make sure that it continues to be possible to override the low-level services (cache backneds, k/v stores) in settings.php
Here is the flow we are aiming for, showing what is needed throughout the flow:
bootstrap configuration
instantiate the kernel
boot the kernel
intialize the container
NEEDS: class name of compiled container
if not compiled
NEEDS: list of enabled modules and their locations
register module namespaces
register bundles
build container
register all services including ExtensionHandler (passing it the list of modules)
bootstrap code
[write container to file if necessary]
profit!
Of the three things the kernel needs to initialize the container, one is from config (the list of module names, which we could just get by parsing the yaml file), and two are from pluggable backends, and this is where the problem lies. The k/v depends on the container, and the cache backends are supposed to be going that way too.
So the options are:
- Move these things (the list of module locations and the container class name) to not be stored in pluggable backends
- Instantiate these stores pre-container and inject them into the container as synthetic services
- Retain the bootstrap container for just these services
Proposed resolution
It should be fairly trivial to find a place to store the container class name that does not rely on a pluggable cache backend. This leaves just the list of module locations, currently in the state k/v store.
Remaining tasks
Figure out whether the list of module locations has to stay in the state k/v store and if so, work around this using either option 2 or option 3 above.
User interface changes
N/A
API changes
TBD
Related issues
#1331486: Move module_invoke_*() and friends to an Extensions class
Original Description
The "bootstrap container", an ugly concept to begin with, is just growing and growing as more things that are required during early bootstrap, (pre-kernel instantiation) are being turned into services in the DIC.
What constitutes "early bootstrap" needs to become a very tiny set of tasks.
Originally, the only reason we needed to bootstrap up to database-level before kernel instantiation was because the kernel itself needs to call system_list() to get the list of enabled modules, seeing as they might be supplying bundles that needed to be registered. Otherwise, bootstrapping to configuration level would have been enough.
There has been discussion around this in other issues, notably #1759152: Add a database service to the DIC and #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache but it crops up elsewhere too, e.g. #1269742: Make path lookup code into a pluggable class.
Let this be the place where we figure it out once and for all.
Comment | File | Size | Author |
---|---|---|---|
#84 | upgrade-path-foo.patch | 1.72 KB | tstoeckler |
#76 | 1784312.unbork-bootstrap.76.patch | 48.16 KB | katbailey |
#76 | interdiff.txt | 918 bytes | katbailey |
#63 | 1784312.unbork-bootstrap.63.patch | 47.95 KB | katbailey |
#63 | interdiff.txt | 959 bytes | katbailey |
Comments
Comment #1
chx CreditAttribution: chx commentedIMNSHO the solution would be a 'push' based approach where a module enable/disable creates the DIC and that's it, one DIC.
But this creates a registry-like nightmare where the loss of a module or the movement of it causes Drupal to go down with a fatal.
Due to PHP throwing uncatchable fatals instead of Exceptions, this is not easy to solve. But, I think, one thing we can do is to redirect to a rebuild (I was even thinking of reusing install.php, think of it) from the classloader if we are to return FALSE. Except that class_exists triggers the same classloader and I am not sure we want to peek into the backtrace to figure out whether the autoloader is fired for a friendly class_exists check or a 'hostile' situation (class foo extends bar, bar tries to load, moved away, kaboom). Perhaps a drupal_class_exists should be used which marks somehow that a false is ok to return? There are only 12 class_exists calls in Symfony and Twig involving a variable class name, 5 of them in some sort of a Loader. We might be able to get away with mandating a drupal_class_exists indeed and do the redirect to a rebuild in case of trouble.
I am just rambling aloud.
Comment #2
Crell CreditAttribution: Crell commentedI don't think DIC rebuilding is the issue here; DIC instantiation is.
Is there a reason we can't just cut/paste most of index.php pre-DrupalKernel-instantiation into DrupalKernel::boot() (or init(), or whichever is appropriate), then let the chips fall where they may and refactor from there? That is, the startup routine is:
1) Load config.
2) Create Kernel.
3) Init kernel (which starts the DIC).
4) Do anything else useful.
5) Handle a request as normal.
Comment #3
msonnabaum CreditAttribution: msonnabaum commentedWe more or less fixed this already in #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache, but I like the idea of getting this in independently of that issue.
Attached patch is just the changes beejeebus and I made to fix the bootstrap issue.
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedLooks like an improvement to me.
Comment #6
katbailey CreditAttribution: katbailey commentedIt's an improvement, but it's still not quite what we need because that bootstrap code is still running before the DIC gets built.
Comment #7
Crell CreditAttribution: Crell commentedCan't we initialize the DIC first, then call drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE)?
Comment #8
katbailey CreditAttribution: katbailey commentedWe can, but we need to get around calling system_list() in the kernel's build method. So, just for the sake of separating out the dependencies, here's a PoC patch that adds a systemList() method to the kernel and then allows us to build the DIC before the call to drupal_bootsrap(). I'm having some bizarre test failures with it that I can't figure out, but anyway, it's really just for discussion purposes.
Please note that I am not suggesting we actually do this, because #1331486: Move module_invoke_*() and friends to an Extensions class is where that should get done (we would then call the systemList() method on the Modules class or whatever it will be called). I might take a pass at that this weekend.
Comment #10
katbailey CreditAttribution: katbailey commentedHuh, ok that is pretty alarming - I wasn't expecting so many test failures. Clearly I do not have a solid grasp on what is going on here :-/
Comment #11
sunOne major question I have is:
AFAIK, the container that is being compiled by the Kernel gets written to an arbitrary filesystem location via PhpStorage. Which makes sense, because most of the services in there are defined dynamically; i.e., they depend on modules/bundles being enabled and so on.
However, I think we want and need the low-level services defined in the early/minimal bootstrap container to be static and editable. I.e., I need to be able to replace and customize the configuration system, state storage, database, and cache backend service definitions before Drupal attempts to use them in any way, so as to prevent it from attempting to read or write anything from or to a storage/backend that must not be used in my setup.
In light of that, I really liked the idea of having a precompiled container in a
[site]/services.yml
as mentioned in #1703266-5: Use a precompiled container for the minimal bootstrap container required outside of page requests, since it essentially serves as a replacement for wonky global variables defined in settings.php and gets rid of them by using explicit service definitions. Unless manually prepared and provided already, this container would be initially written by the installer (and install profiles/distros would be able to have their say), but afterwards left alone.Comment #12
katbailey CreditAttribution: katbailey commentedSo, one thing I definitely think we should get rid of is this idea of defining the config services in that "bootstrap container" and merging them into the main container. I introduced that fugliness in the bundles patch merely to avoid defining the same set of services in two places. However, the container as defined in drupal_container(), if we solve this issue properly, should only ever be used when either a) there is no kernel or b) an exception is thrown during kernel instantiation/boot.
I definitely think we should eventually have a precompiled container for that and so how the config services are defined there does not need to be the same as how they are defined for normal page requests.
Comment #13
pounardCore is a Symfony bundle or not? If so, it can define those "pre-bootstrap" services as normal DIC services, which will be compiled as in any SF site. Sites could define overrides via their own bundles (profile bundle, module bundle, or just some config file somewhere). If I recall correctly, those overrides are possible and legit in Symfony, so why not for us? If we consider this as being true, we don't need any "pre-bootstrap container", we just have a core bundle whose configuration can be overriden in many ways quite easily (compiling doesn't matter here, it's just an optionnally added behavior to the DIC but which should not matter to business). The only requirement for bootstrapping normally just like any SF2 site in this context is having the system list pre-defined somewhere instead of into database so that we can load bundles without requiring database (or at least the profile and core bundles, and the path to potential per config overrides).
Comment #14
catchIf we write the container to disk, then it should not be necessary to call system_list() to register bundles each request, since we'd only need to do that when compiling the container no? I didn't get to properly review the bundles patch before it went in and I think I need to sit down with it a bit more after the fact now..
Comment #15
pounardTrue, but in that case we still need the database service in order to load bundles, which kinda makes a circular dependency.
Comment #16
chx CreditAttribution: chx commentedIf we write the compiled DIC to disk on module change then we are good. I do not understand #2 which is supposed to be an answer to my rant? How is DIC instantiation is a problem once it's compiled? Once it's compiled if I understand things correctly you do not need to have bundles, that's just the build phase.
See #1 for more.
Comment #17
pounardIMHO we should not even consider the compile phase exists when we deal with this sort of bugs. The normal behavior is making it work gracefully without, then if it works, the compiled version will follow gracefully, since it's just supposed to be a decorator pattern around the DIC that is tied to performances only but that should never ever alter anything else than performances.
If we leave out the compiled DIC, needing the database to load bundle might be some kind of circular dependency because we need to 1) load core bundle 2) build the DIC in order to bootstrap and use the database 3) load all other bundles that will alter the DIC (sadly too late because it's already have been used).
Comment #18
sunHm. Is my question in #11 deliberately ignored? ;)
Once more, shorter: How do you guys plan to swap out e.g. the config service definition, before the system attempts to get the service?
(PhpStorage cannot be edited, since it actively prevents edits. Global variables cause all kinds of pain and hinder testability.)
Comment #19
pounard@sun Your proposal of having pre bootstrap services is I think the right way to go, but I'd call them "core bundle's services" which could ideally be overridable using either another config file (Yaml or PHP or anything else, since SF allows us to use all that) or another bundle.
EDIT: Actually I re-read your proposal, which is to have two DICs, one which is not compiled but built using a Yaml file, and another which is the one being compiled. I think this is bad and adds too much complexity.
Comment #20
catchYes even if we move modules to CMI then there's still going to be a circular dependency (assuming CMI is in the DIC) and it makes me uncomfortable.
The issue is that we want to register stuff into the DIC, and the registration process itself depends on services registered to the DIC.
If we want people to be able to dynamically add and remove bundles from with Drupal (i.e. via enabling/disabling modules but really via any other mechanism that's not hand-editing a PHP or YAML file somewhere), then that's going to be there in some sense. I'd really like to be removing circular dependencies from core rather than adding them at this point, so I'm bumping this to critical.
Comment #21
katbailey CreditAttribution: katbailey commentedThis is where "synthetic" services come in handy - services that are not instantiated by the DIC but are injected into it. The DrupalKernel would instantiate a db connection, which it needs in order to register module bundles. It then registers a synthetic "database" service and sets it using the connection it has already instantiated. This is essentially the approach I'm taking in #1331486-27: Move module_invoke_*() and friends to an Extensions class.
Comment #22
andypostI like @sun idea about 2 phase bootstrap, my vision on this is that Bundles should contain only code that could work without "module.inc" so applicable for early bootstrap.
A good example is #1161486: Move IP blocking into its own module module should work different depending on database class availability so could fire on early bootstrap and also on full.
So having a bundle-set-for-bootstrap that uses a pre-compiled container is a good idea, this can load a module system if it needed to serve a current request.
Comment #23
Crell CreditAttribution: Crell commentedRefiling.
Comment #23.0
katbailey CreditAttribution: katbailey commentedUpdating the issue summary
Comment #24
katbailey CreditAttribution: katbailey commentedAfter encountering a circular dependency mess in the ExtensionHandler issue #1331486: Move module_invoke_*() and friends to an Extensions class due to the kernel needing the state k/v, which in turn depends on the container, I figured it was time to separate these issues out again. I think we can solve the bootstrap mess without having the ExtensionHandler in place - I've updated the issue summary here to reflect where I think we're at and what we need to do.
Comment #25
chx CreditAttribution: chx commented> Move these things (the list of module locations and the container class name) to not be stored in pluggable backends.
Use drupal_php_storage for these because it is not relying on anything (like a database) and is fairly fast (definitely faster than a YAML file parsing).
Comment #26
katbailey CreditAttribution: katbailey commentedSo, chx has been doing stellar work in getting the really hard part of this figured out. The patch over at #1831350: Break the circular dependency between bootstrap container and kernel is RTBC. Once that is in it's a pretty minor task to actually make the desired change for this issue, i.e. to only bootstrap up to config before booting the kernel. I've rolled a combined patch for this (i.e. the changes required for this on top of chx's patch) so as to see what testbot makes of it and whether I am missing anything. Adding the patch for this issue as a do-not-test.patch.
Comment #28
katbailey CreditAttribution: katbailey commentedUgh, yes I am indeed missing something. Registering new bundles on module enable does not work with this patch. Looking into it...
Comment #29
katbailey CreditAttribution: katbailey commentedOK, let's try this...
Comment #31
katbailey CreditAttribution: katbailey commentedWhee!
Comment #33
katbailey CreditAttribution: katbailey commented#31: 1784312.unbork-bootstrap.31.patch queued for re-testing.
Comment #35
katbailey CreditAttribution: katbailey commentedLooks like this is my race condition friend again - the exceptions were in completely different tests this time and 0 fails, whereas there was 1 fail the first time. Sigh.
Comment #36
Fabianx CreditAttribution: Fabianx commentedTrying if the old one is suffering from the race conditions as well:
#29: 1784312.unbork-bootstrap.29.patch queued for re-testing.
Comment #37
Fabianx CreditAttribution: Fabianx commented#29: 1784312.unbork-bootstrap.29.patch queued for re-testing.
It does, before it had only 1 fail, now it has much more, several exceptions and also a warning that mkdir already exists.
Comment #38
Fabianx CreditAttribution: Fabianx commentedThis is a bot-only patch that contains #1708692-79: Bundle services aren't available in the request that enables the module inside as multi-patch.
Comment #40
katbailey CreditAttribution: katbailey commented#38: core--stop-too-much-kernel-bootstrap--1784312--38.diff queued for re-testing.
Comment #42
sunSome changes in this patch look suspicious to me. As agreed on in #1831350: Break the circular dependency between bootstrap container and kernel, I demand that the essential DrupalKernel test coverage from #1774388: Unit Tests Remastered™ is committed first. Afterwards, we can happily revise DrupalKernel in whatever way we want.
Comment #43
chx CreditAttribution: chx commentedAdd class_loader and php_storage as synthetic services and switch php storage to PhpStorageFactory (poor drupal_php_storage function, only lived three months).
Comment #45
chx CreditAttribution: chx commentedMoral of the story: before posting a patch run a WebTestBase too, I only ran unittestbase based ones...
Comment #47
katbailey CreditAttribution: katbailey commentedMinor change to get rid of the non-existant php_storage service problem...
Comment #49
chx CreditAttribution: chx commentedNot so minor change: php storage consumers now keep their own objects instead of static or static-like cache (yuck).
Comment #51
chx CreditAttribution: chx commentedComment #52
Dries CreditAttribution: Dries commentedsun: please be careful with words like 'I demand' (see #42). You don't have special privileges to demand things. It goes against our collaborative nature. It's really important that we maintain the right culture. Thanks.
Comment #53
Berdir#51: 1784312_51.patch queued for re-testing.
Comment #55
chx CreditAttribution: chx commentedWell, we no longer merge in the bootstrap container. Nor we need to IMO. Just leave the kernel alone with this test.
Comment #57
Lars Toomre CreditAttribution: Lars Toomre commentedAttached are some nit-pickitty observations from reading through the patch in #55. Perhaps these can be included if a re-roll is performed?
My understanding from [#1354] is that this should be 'Contains' instead of 'Definition of'. Same elsewhere in the patch.
Can we add a type hint here? 'string'?
This type hint should start with a '\'.
Appears that this method is missing a docblock.
Any reason why use statements are organized like this? Makes little sense to me.
Should include the default value like 'Defaults to TRUE.'.
Appears we are missing a @return directive. Not clear from just reading patch.
Surprised to see this class defined as a constant. What happens if we want to use a different Class Loader?
Appears we are missing a @param directive for $module_paths.
Could use a blank line before use declaration.
This change makes the conversion of variables to state system harder. Perhaps include the conversion of those variables here? Or at least cross-reference the other issue here?
Missing a one-line description.
Comment #58
chx CreditAttribution: chx commented> Surprised to see this class defined as a constant. What happens if we want to use a different Class Loader?
It's a synthetic service, it just sets expectations for the container, an object (of whatever classloader extending class we want) is set later. Added a comment to the container->set to make it easier to see.
> This change makes the conversion of variables to state system harder.
It's still search/replace of variable_set to state()->set
I addressed the rest and hopefully fixed the tests. Also, we get the useful feature of adding bundles from settings.php (possibilities include a SiteBundle for site specific things that are not module bound, perhaps a theme specific bundle or two w the parent etc). This was discussed at length w Crell.
Comment #59
g.oechsler CreditAttribution: g.oechsler commentedI found a few little things. Beside that it looks fine, although I can only "parse" it as I'm not fully understanding what it does and why it's done like this.
Typo in @return.
Change of parameter name should be in comment as well.
I can't see why PhpStorageFactory is used here.
Comment #60
chx CreditAttribution: chx commentedRerolled.
Comment #61
katbailey CreditAttribution: katbailey commentedComment #62
effulgentsia CreditAttribution: effulgentsia commentedThis is such an awesome step towards cleaning up our bootstrap process. It all looks good to me, except:
Unless someone can explain how this is not a bug, can we at least add a @todo here to follow up on this?
Comment #63
katbailey CreditAttribution: katbailey commentedAdded a todo and created this follow-up issue: #1846376: Namespaces for disabled modules are registered during the first request after a module is disabled
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedThanks.
Comment #65
sunI actually stumbled over that part, too.
Given the current state of plugin system discovery, which relies on the classloader's registry of namespaces, I do not think that it is a good idea to introduce that kind of "mystery." That's sorta guaranteed to cause havoc down the line for other, innocent core contributors.
Comment #66
katbailey CreditAttribution: katbailey commented/me waits with baited breath and fingers crossed for sun to say that was a cross-post :-P
Comment #67
sunUnfortunately not... Due to the issue referenced in #65, this detail of the proposed change here can realistically have terrible consequences.
There's no test being added here that proves it does not. (Rather the contrary; existing test coverage that ought to protect us from bad things from happening is adjusted in a way that seems to make the entire test obsolete, because it doesn't really test anything special anymore.)
Comment #68
chx CreditAttribution: chx commentedThis needs to go in so we can remove the classloader's reliance on namespaces and instead make it rely on kernel parameters.
Comment #69
chx CreditAttribution: chx commentedRewording: so we can remove the plugin system relying on the classloader storing the namespaces. Instead, the kernel / container can store them.
Comment #70
Crell CreditAttribution: Crell commentedI just spent the last hour and a half or so staring at this patch. I think there's a variable name I could quibble about, and a sentence that requires an extra comma in it, but I won't tell you where they are because that might delay this patch getting committed. :-) This is a huge step forward, although admittedly not the last one, and I think unblocks a number of other issues either directly or indirectly.
I believe sun's concerns are unfounded. As chx notes, this lets us put the active module list and the active namespace list in the DIC, where they are 1) As fast as humanly possible to access; 2) decoupled from the actual classloader implementation itself. That is, far from having "terrible consequences" for the plugin/classloader dependency, it resolves it in a clean and performant fashion.
Comment #71
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is one step in the right direction. But, after this patch, the bootstrap reads:
... which is still very problematic. The main issue is that the request is actually implicitly used by
DRUPAL_BOOTSTRAP_CONFIGURATION
to load the proper configuration folder. What we most likely want is something like this:And in addition, we also want a way to create a kernel outside of a request (which is completely impossible right now, but necessary for, for example, Drush), with something like this:
Comment #72
chx CreditAttribution: chx commentedYou are right but that's a followup.
Comment #73
Damien Tournoud CreditAttribution: Damien Tournoud commentedI never said the contrary. I'm fine with this as a step in the right direction.
Comment #74
effulgentsia CreditAttribution: effulgentsia commented#63: 1784312.unbork-bootstrap.63.patch queued for re-testing.
Comment #76
katbailey CreditAttribution: katbailey commentedHopefully this one will be green...
Comment #77
chx CreditAttribution: chx commentedJust a reroll to keep up with HEAD putting back in the RTBC bin.
Comment #78
Crell CreditAttribution: Crell commentedSince this is a touchy patch.
Comment #79
Crell CreditAttribution: Crell commentedEep.
Comment #80
webchickThis has catch written all over it. :)
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedI don't want to mess with the RTBC status of this issue, so instead, I posted a fix to the ClassLoaderTest regression to #1846376-1: Namespaces for disabled modules are registered during the first request after a module is disabled.
Comment #82
catchDidn't see anything much to complain about so I've gone ahead and committed/pushed this one. Hopefully we can continue to clean this all up but this feels like we got a bit further.
I'll unpostpone #1846376: Namespaces for disabled modules are registered during the first request after a module is disabled.
Comment #84
tstoecklerSorry, for re-opening this, but this issue introduced a BootstrapConfigStorageFactory which I don't understand at all. It references $conf['drupal_bootstrap_config_storage'] but is the only place in core to do so. I also don't see that part discussed in this issue at all.
I'm hitting this in the D7 -> D8 upgrade path. BootstrapConfigStorageFactory is called from DrupalKernel::registerBundles which is called (a couple levels up in the stack) from DrupalKernel::boot() right in update_fix_d8_requirements(). Most importantly this is called before checking if settings.php exists. Excerpt:
Therefore in BootstrapConfigStorageFactory the call to config_get_config_directory() happens with empty global $config_directories and everything blows up. I have no idea how the upgrade path even remotely works at this point (it doesn't for me locally). The reason I am not opening a new issue is that I am not able to make any sense at all of BootstrapConfigStorageFactory.
The attached patch resolves the fatal at least, but update.php still doesn't spit anything out.
Comment #85
jhodgdonPresumably we do not need the "avoid commit conflicts" tag on this reopened issue?
Comment #86
catch@tstoeckler would you mind opening a new bug report for this?
Comment #87
chx CreditAttribution: chx commentedAnd if you do assign it to me...
Comment #88
chx CreditAttribution: chx commentedContinued at #1943726: BootstrapConfigStorageFactory and update.
Comment #89.0
(not verified) CreditAttribution: commentedUpdated issue summary.