This is a branch off of #550124-22: Remove prepared statement caching, where this was found. Quoting:
I also discovered in the course of testing a serious OMGWTFBBQ. The fatal error in question is an "Exception thrown without stack trace" bug that appears if and only if we explicitly set prepared statement emulation to FALSE (which is the default) and we cache prepared statements. If we don't cache prepared statements, it doesn't die. Erk. So I stuck a print statement into _drupal_check_registry() to see if I could figure out where it's dying. That gave me plenty of debug output, most of it "headers already sent", but then of all things I get a fatal error calling _system_rebuild_theme_data() from theme.inc, out of the maintenance theme pathway. Because that function is part of the core of the theme rebuild process, yet lives in system.module, which is wrong. I tried moving it to theme.inc, which would make sense, but then I get a fatal on system_rebuild-theme_data(). That in turn calls system_get_files_database() and system_update_files_database(), which are not part of the theme system. So while trying to benchmark PDO, I've found a trainwreck of stupidly factored code scattered about system.module (as if we don't have enough of that already) that may or may not be causing PDO to crash. I have no idea how or why.
I have no idea why database configuration would trigger it, but the ass-ackwards comingling of theme.inc and system.module is a bug, no ifs-ands-or-buts. I am not sure that we can do anything about that for Drupal 7 other than not use that database configuration, but we really, really need to get better about not writing suck godawful spaghetti. Circular dependencies are almost never a good thing.
Comments
Comment #1
sunThis sounds like a critical clean-up is required.
Comment #2
catchThere needs to be a demonstrable bug before this is critical. Fixing system.module spaghetti isn't a D7 task.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedsystem module holds hook implementations for every file thats in /includes because those can't have hooks. for example, system_cron() deletes fils in the temp dir. that should be in a file_cron which lives with other file code. so, for d8 i would propose a radical refactoring:
- get rid of most or all of /includes.
- put that code into new modules that live in a new modules/framework directory
- move the current stuff in /modules to /modules/cms.
obviously this idea needs work, but i think its a good start.
Comment #4
Crell CreditAttribution: Crell commentedI wouldn't necessarily get rid of all of /includes. But yes, the complex inter-dependency between /includes and system.module is a problem. I'd rather see them cleanly separated (so that nothing in /includes depends on system.module to run, although it may to be useful), and system.module broken up. Eg, theme.inc should not have *any* theme functions in it. Those should all move to a module, either system or something like it. But the theme rebuild process itself should live entirely in theme.inc (or at least in /includes).
That's definitely a Drupal 8 refactoring, though.
Comment #5
andypostsystem.module requires formalization, as a minimum separation of cms and framework parts. Otherwise, the future of new api will cause even more spaghetti code. I think that the movement of several functions between files is possible in the current branch.
First need to identify parts that are available at different stages of bootstrap. It may be easier to create system.inc and put inside all functionality which is available before DRUPAL_BOOTSTRAP_FULL (a framework part, I suppose)
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribing.
Comment #7
catchI don't see "moving code around" patches getting into D7, or really making much difference at this stage if that's all they do, so moving this to D8. This should be one of the first tasks once D8 opens up though.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedamen to this effort. as well as the bigger ideas here, i'd like to see other system.module refactoring in D8 as well:
- kill {system} and replace it with {theme} and {module}, and stop storing the path to a module's .module file
- clean up the way we rebuild module and theme data. reparsing the known file-system universe to find changes should not be required when we install a new module/theme. the $enable_dependencies hack for module_enable() has to die
Comment #10
sunBoth items in #9 are slightly off-topic, but I especially disagree with this one. Modules, themes, installation profiles are becoming more and more the same, all extension types participating everywhere. Hence, splitting types off from {system} sounds wrong to me, as we're about to lose the separation in the first place. But as mentioned before, that's off-topic here and we should discuss elsewhere.
Comment #11
catchDowngrading all D8 criticals to major per http://drupal.org/node/45111
Comment #12
catchDrupal 8 is nearly here and I'm feeling murderous. #1008166: Actions should be a module
Comment #13
catchClearing up common.inc needs to be part of this overall effort as well. #1018602: Move entity system to a module is a start.
I don't have a complete plan for this yet, but here's the general direction:
* Anything in system.module that could be handled by an existing core module should move to that module where possible, or for some things create a new one.
* Any functionality in includes/* that system module implements hooks for, especially when it won't work without those hook implementations, should probably be a module.
* Any include that is hard coded into _drupal_bootstrap_full() should probably be a module too.
* Check common.inc and theme.inc for stuff that could be moved into modules (like the default theme_* functions).
* After this, we should only have system implementing some hooks on behalf of whatever is left in includes/* (mainly hook_schema()), and providing a UI (like admin/modules etc.) - decide if any of this functionality ought to be split out or not.
* While going through this it'll be unavoidable, but the process should show us which includes and modules depend on (or are enhanced by) which, which is currently impossible to figure out without manually reviewing code, because it all bypasses the dependencies system by being required and/or hardwired into bootstrap.
* It's a separate effort in terms of flipping the switch, but we should be able to make every module work if it's set as optional.
* It would be good to remove the circular dependency between system module and the module system (system_install() defines the system schema, you can't install a module without the {system} table). That's not a case of just moving a few (thousand) lines of code around though.
Comment #14
sunNice summary @catch - I think I fully agree with all steps you listed.
Comment #15
catchStarted looking through a bit more:
#1033410: Move token.inc to a module
Tokens is a good example. The only time we actually use token replacement in core is when sending e-mails - user registration and system mail actions (although I'm sure there's other places they could be used but aren't yet). There's lots of hook_token_info() implementations but those don't require token to be installed or loaded at all.
So moving token.inc, system.tokens.inc and the tests to a module would be fine. We'd need to make it a dependency of user.module, because that calls token_replace(), but this is a problem we have in terms of user module handling basic user stuff along with a full infrastructure for sending e-mails etc. which is not for this issue.
No issues for these two yet, but other candidates:
All the file_managed stuff in file.inc - currently the only module that does anything with file_managed (apart from file and image modules) is user module for user pictures. We could move those functions to file.module or file_managed.module, move system_entity_info() into there, and that would further slim down system module.
Also tablesort.inc, this is a couple of helper functions and a fairly large class at the moment. This could move to a module (possibly a generic 'elements' module), with the class in a .inc and loaded by the registry. It's senseless to load something that is used only a fraction of pages for every request like this.
Comment #16
sunMy plan for D8 is to turn Form API into a form.module.
Given that change, and current efforts to introduce a #type 'table' in Edge that combines support for most of the table* features, I'd agree that it's time for an element.module (singular). Not only moving system_element_info(), but potentially also all of the element_* functions in common.inc.
Comment #17
catchBoth of those sounds great :)
Comment #18
catchAnother one #1033818: Move xmlrpc includes into xmlrpc.module.
Comment #19
catch.
Comment #20
sun.
Comment #21
catchNot strictly system.module, but two oddities in /includes are tablesort.inc and pager.inc - both contain a handful of procedural functions and a class, and both are loaded indiscriminately in _drupal_bootstrap_full(). At least, the classes could move to registry-loaded includes, and the functions could still live somewhere accessible. element.module would be good if they were actually elements.
Comment #22
XanoI'd love to keep System.module so we can get rid of /includes and move all those files to /modules/system/includes to improve consistency in core, even though I agree we can move parts of System.module elsewhere.
Comment #23
catch#1120928: Move "history" table into separate History module
Comment #24
catch#1161486: Move IP blocking into its own module.
Comment #25
catch#1167144: Make cache backends responsible for their own storage
Comment #26
catch#1033818: Move xmlrpc includes into xmlrpc.module
Comment #27
podaroksubscribe
Comment #28
plachComment #29
joachim CreditAttribution: joachim commentedsubscribing :)
Comment #30
ksenzeeSubscribe.
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedhallelujah...needed to be done quite some time :)
Comment #32
catchMarking duplicate of #1224666: [meta] Unofficial Drupal 8 Framework initiative which is a bit more structured.