Problem
- The only procedural code that exists in Drupal 8 modules and installation profiles are hook implementations.
- Not every module/profile has a use-case for implementing hooks.
- But yet, every module/profile requires a .module/.profile file.
Goal
- Make .module/.profile files optional.
Proposed solution
- #2188661: Extension System, Part II: ExtensionDiscovery laid the ground work by discovering all extensions through their
.info.yml
files only. -
It also introduced a new
Extension
class, which takes three arguments:$type
: The extension type; i.e., 'profile', 'module', etc.$pathname
: The pathname of the extension's.info.yml
file.$filename
: The name of the main extension file; e.g., 'node.module'.
-
→ Simply make
$filename
optional. -
To make that possible, the module list used by
ModuleHandler
andDrupalKernel
is replaced with a list of actualExtension
objects; i.e.:$module_list = array( 'node' => 'core/modules/node/node.module', 'migrate' => 'core/modules/migrate/migrate.module', 'testing' => 'core/profiles/testing/testing.profile', );
…becomes the following in
DrupalKernel
:$module_list = array( array('type' => 'module', 'pathname' => 'core/modules/node/node.info.yml', 'filename' => 'node.module'), array('type' => 'module', 'pathname' => 'core/modules/migrate/migrate.info.yml', 'filename' => NULL), array('type' => 'profile', 'pathname' => 'core/profiles/testing/testing.info.yml', 'filename' => NULL), );
…which actually are the
Extension
class constructor arguments, which in turn means:$module_list = ModuleHandler::getModuleList(); var_dump($module_list);
array( 'node' => object(Drupal\Core\Extension\Extension)[28] protected 'type' => string 'module' (length=6) protected 'pathname' => string 'core/modules/node/node.info.yml' (length=33) protected 'filename' => string 'node.module' (length=12) ), 'migrate' => object(Drupal\Core\Extension\Extension)[28] protected 'type' => string 'module' (length=6) protected 'pathname' => string 'core/modules/migrate/migrate.info.yml' (length=33) protected 'filename' => null ), 'testing' => object(Drupal\Core\Extension\Extension)[28] protected 'type' => string 'testing' (length=6) protected 'pathname' => string 'core/profiles/testing/testing.info.yml' (length=33) protected 'filename' => null ), )
→ which, in turn, provides access to
Extension
class methods to all code. DX++
.
.
Original summary
yes, that's right, no more requirements for .module files.
summary of agreement from #drupal discussion, mainly between Crell and webchick:
1. From now on, only .info is required to dictate the presence of a module. .module is optional.
2. Any code in .module will always be loaded on all pages.
3. Any code not in .module will be lazy-loaded
4. Nothing changes to the files[] declaration
5. Well-document all the reasons to move code out of .module, even to the point of .module being absent.
as for what to put in a .module file, Crell summed it up as:
"If you have a constant or function that other modules will need to call explicitly, put it in .module. For *everything else you can think of*, put it in a separate file."
this patch will just make the .info and .module dependency changes, and we can do the rest later.
Comment | File | Size | Author |
---|---|---|---|
#191 | 340723.191.patch | 56.86 KB | alexpott |
#191 | 186-191-interdiff.txt | 1.01 KB | alexpott |
#186 | extension.optional.186.patch | 55.85 KB | sun |
#182 | interdiff.txt | 8.87 KB | sun |
#182 | extension.optional.182.patch | 55.38 KB | sun |
Comments
Comment #1
swentel CreditAttribution: swentel commentedI like the idea, that's what I've been experimenting with the last two weeks. For example, I moved away hook_menu, hook_cron & hook_cron from the module file to a module.hook file since these are only loaded on few occasions. Not sure if that's a good idea though, only trying to brainstorm along with this idea.
Comment #2
Crell CreditAttribution: Crell commentedThat's exactly the idea, swentel. We need to establish some common patterns in core, but any hook, any callback, and any class/interface can be lazy loaded as needed and so does not need to be in .module. For many modules, that means no .module file so the total code weight is 0.
Comment #3
catchSubscribing. Seems like the first thing we need to decide is whether we want lots of includes or few. If a module implements 10 hooks, and each one is likely to be called uniquely on a page view, do we go so far as to do an include per hook?
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commented@catch: lets try to avoid generalising on this question until we get rid of the .module requirement and start to port some core modules.
i think we should see some obvious divisions fall out when we start implementing it.
Comment #5
catchJustin: completely missed "this patch will just make the .info and .module dependency changes, and we can do the rest later." - of course!
Comment #6
chx CreditAttribution: chx commenteda small problem, though. themes have .info files as well, how will you make a difference? based on where they are? I guess that works.
Comment #7
Crell CreditAttribution: Crell commentedYep, the directory they're in is fine.
So who's going to roll a patch already??? :-)
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm busy til the weekend, so i'll roll a patch then unless anyone else beats me to it.
Comment #9
swentel CreditAttribution: swentel commentedOk, a first test. It ain't the prettiest code I guess, but patch also has a proof of concept where I renamed php.module to php.inc. All tests still pass after this (and if not, apply 328781 first).
Comment #10
swentel CreditAttribution: swentel commentedAnd now with patch
Comment #12
chx CreditAttribution: chx commentedCare to write up why you are doing work in drupal_load ? That strikes me as a bad approach performance wise.
Comment #13
axyjo CreditAttribution: axyjo commentedSubscribing.
Comment #14
swentel CreditAttribution: swentel commented@chx, it's a disaster performance wise, it was merely a proof of concept.. I'll ping you guys for a brainstorm on irc next week to see what the best approach would be for this.
Comment #15
aaron CreditAttribution: aaron commentedI know it's getting slightly ahead, but is there a registry (or plans) for consistent lazy loading, so one could dictate what files for what hooks?
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedi won't be getting back to this for a few weeks, moving to another country is time consuming.
Comment #17
mfer CreditAttribution: mfer commentedsubscribe
Comment #18
BerdirOk, attached is a slightly different approach, still a bit hacky though.
Instead of always storing the .info file, my patch only stores the .info file instead of the .module if no .module file exists. So, if one exists, everything will work exactly as before.
Instead of changing drupal_load(), I changed module_load_all(), which now uses a query instead of module_list() and only executes drupal_load for modules which provide a .module file. This part is still a bit hacky. I'd love to get rid of that additional query but module_list currently doesn't give me the stored filename of a module. The second hacky part is that I have to use strpos on the filename to check if it is a .info file - A flag in {system} would be nicer.
A follow-up improvement could be to remove the file_exists check in module_list (configurable, of course), so that it doesn't need to check the filesystem for every module, when it only contains some hooks which aren't loaded on most requests.
Comment #20
BerdirOk, that one was ugly to debug.
My patch removed the explicit module_list(TRUE) call and because of that, the module list wasn't rebuilt before the init hook was executed and some needed files weren't included. Re-added that call for now.
Comment #21
BerdirPatch imcomplete...
Comment #22
Dries CreditAttribution: Dries commentedI'd like to get a good motivation for this patch. What is the reason for trying to do this? Once I understand why this is a good thing, I'll proceed reviewing the code.
Comment #23
BerdirThe main idea is that small modules, that provide only hooks (which are maybe only used on very few requests) don't need to provide a .module file, which is included and parsed on every request. The registry will take care to load the code only when we really need to execute those hooks.
php.module is already converted as part of this patch and a great example for that use case. It contains only 3 hooks which aren't used for "normal" user (well, I hope that! :) ).
BTW: I don't think that it is ready for your review, though :) I just tried to get it somehow working, it still needs to be improved, I don't like the code in module_load_all().
Comment #24
Crell CreditAttribution: Crell commentedDries:
The background problem is this: With the registry, anything that can be called indirectly can be lazy-loaded, which is good for performance. However, some functions (not classes, since they can always lazy-load) you really do want to assume will always be available, eg, node_load() and other API-esque direct functions.
So the question is how do we ensure that those are always loaded while minimizing the amount of code we load that we don't need. The solution we worked out is:
1) We still load .module files, always, so you can always assume something defined there will be available.
2) Those modules that do not have any such code that is not possible to lazy-load can simply skip their .module file, which means the module has zero performance impact unless one of its hooks is actually called.
3) That necessitates making it possible for a module to "exist" based just on its info file, so that we can have modules that are all-lazy-load.
4) Enter this patch. :-)
I've not reviewed it yet, but that's the idea behind it.
Comment #25
Dries CreditAttribution: Dries commentedThanks for explaining. That seems to make sense, and I agree that this is a useful feature.
The changes to module_load_all() are a bit messy so let's work on that. We also seem to introduce an additional SQL query which seems to defeat the purpose of this patch. Code needs more work.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedtagging with Registry
Comment #27
BerdirOk, let's try this again :)
I changed the array format returned by module_list to filename => name, instead of name => name. This requires a few tweaks in the tests and module_exists(), it seems to work fine, but I didn't run all tests.
With that change, we don't need the additional query in module_load_all. I also reverted the logic from "if .info, don't include" to "only include if .module"
Comment #28
RobLoachNote that drupal_get_path() will need to be revamped as it currently requires that a {name}.{type} file exists. Instead, it should look for a {type}s/{name} directory.
In #338002: Allow modules to cache module information, I attempted to make module_list() return name => info array. This would mean that from every module, you could have easy access to all the data that's in the .info file. Since then, the registry went in so there might be a better way around it.
Comment #29
Berdir- as discussed, drupal_get_path() will still work, because drupal_get_filename knows the filename of every module.
- While discussing this with Rob Loach, I found out that there is a way easier and also better way, we can handle it solely in drupal_load(), as that needs to load the filename anyway. It also works when a module is installed/enabled and so on.
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commentedat the very least, i think we need some comments around this:
but, if we no longer store a filename unless its a .module, why do we need the check? after this patch, drupal's core module system should never ask drupal_load to load a .info. so i think its a case of 'doctor, doctor, it hurts when i do that' for contrib - they should just not do it.
also, an upgrade path?
it seems HEAD is broken a the moment, so i'm not sure if this patch breaks anything.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops, that should be CNW.
Comment #32
BerdirAgreed ;)
We do store the .info file. But drupal_load() is imho the only place where the difference matters. For example, drupal_get_filename('module', 'php') is still used when you call drupal_get_path('module', 'php). But as we do a dirname() there, it doesn't care which file it actually returns.
It's not only contrib that invokes drupal_load(). According to a.d.o (http://api.drupal.org/api/function/drupal_load/7), that function is also called when a function is installed/enabled/uninstalled. If we change as it was in my old patch, we have to check everywhere where we invoke drupal_load() if it really has a .module file.
To do what exactly? The system table will be updated when system_get_module_data() is called.
Comment #33
BerdirAdded a comment...
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedabout the updgrade path, sorry, i didn't actually test a D6 -> D7 with this patch, so if you have tested that and its all good, ignore my comment.
about the rest - i think we need to fix the fact that the the system table uses the filename as its primary key. it should probably be something like module name + directory, then we can get rid of this 'we have to store a file, but sometimes we can't load it' special case stuff.
storing just one file for a module seems like a hangover from bygone days that we should make a clean break from. when we want to get the directory a module lives in, we should be able to just read that from the table, not have to do dirname(filename).
Comment #35
swentel CreditAttribution: swentel commentedWhy not reintroduce the bootstrap field we had in D5, so it's easier to only load those modules with a .module file. I agree with justin that the check in drupal_load doesn't feel right - I actually tried it too - but I think we can be better programmers right ? :)
Comment #36
BerdirHm..
We *load* (enable) the module.. the only thing we don't do is including the .module file. But we *want* it to be in module_list(), execute hooks of that module and so on...
Comment #37
Crell CreditAttribution: Crell commentedWith the registry, is there even a reason to have the "boostrap" distinction anymore? The reason for it is so that we can load a few modules for their boot hooks early on, then potentially come back later and load the rest. However, with the registry we need only say "run the boot hook" and the registry finds what files that entails, whatever they are (.module or otherwise). Code doesn't load until it's used.
So do we even need to make bootstrap a special case anymore?
Comment #38
sunsubscribing
I disagree with this limitation. We should allow modules to ship with themes. http://drupal.org/project/admin is a nice example of what can be achieved when modules can provide a theme. Currently, that module implements a major hack to achieve this - being the reason for why it does not always work properly.
Comment #39
Crell CreditAttribution: Crell commentedI don't know what "modules can include themes" means. Modules can already include templates, theme functions, and preprocess functions. I do support adding the stylesheets and scripts keys to module info files so that they can ship with CSS and JS more easily, but I don't know if that's what you mean.
Comment #41
lilou CreditAttribution: lilou commentedSetting to 'needs review' - testbot was broken.
Comment #43
BerdirTestbot, I don't believe you :)
Comment #44
BerdirI'd really like to have support for this in D7...
To summarize my opinion...
We *do* want to load modules without .module files, they should part of module_list(), module_exists() and so on... The only difference is that we include the .module file only if it exists. So a bootstrap field is something different. Also, this is not related to the themes/modules issue imho as the limitation is not added by this issue.
While I agree that the system table should store the directory and filename separatly, we still need a check if it is a .info or .module somehow. We could probably allow the filename column to be NULL and only include non-NULL values, this would look a bit nicer. And the check IMHO belongs into drupal_load(), I can't see a better place.
I need to check if I can get this working with a directory column, however, it will require quite a lot more changes.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedberdir - yes, the directory + name for primary key will require a lot more change, but i think its the way to go. i'll help if you post an initial patch.
Comment #47
pwolanin CreditAttribution: pwolanin commentedI really fail to see why this feature is useful.
Comment #48
axyjo CreditAttribution: axyjo commented@pwolanin: Sometimes modules don't need code to be loaded on every single page. However, under the current setup, a .module file must exist in order for Drupal to recognize its presence (even if it's blank).
Comment #49
David StraussThis feature is useful. Sometimes, projects have modules to manage schema updates, and they might only need a .install file.
Also, from an academic architecture perspective, it's correct for us to allow the degenerate case (just a .info file) to work.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #52
Crell CreditAttribution: Crell commentedThis is not won't fix. At worst it's postponed. We can come back to it in Drupal 8.
Comment #53
peterx CreditAttribution: peterx commentedI like direction this discussion is headed. If the load code is changed for D8, could we open a related issue or add a hook to let a module control the loading of optional modules? Your .info change would be in the right area to add such a change. I added a quick description of my thinking for a related change in case this current issue evolves into a change to the hook system.
Is there an opportunity to add a related task to only load modules for a role or a theme? If there are no required entries, it is always loaded. If there are any required entries, it is loaded only for those entries. Perhaps the .info file could have something like the following.
required-role: administrator
Comment #54
glennpratt CreditAttribution: glennpratt commentedGiven the progress on #1299424: Allow one module per directory and move system tests to core/modules/system, I'm getting a head start on a patch built from that. Here's my plan of attack if someone wants to poke holes in it:
Detection
.info
file, not.module
or.profile
Load
file_exists()
oris_file()
at load time, cache hits. Similar tomodule_implements()
cache. Adding a module file where one didn't exist will require cache clear.System Schema
filename
withuri
.info
,.module
and.profile
paths and presence are details that don't need to be in the system table.uri
type
,name
)? Probably out of scope.Pre-update hook
filepath
/ lack ofuri
in the results from system will build the properties as needed to be backwards compatible during upgrade.Comment #55
Crell CreditAttribution: Crell commentedWhen you say replacing filename with uri, do you mean something like #1308152: Add stream wrappers to access extension files? Because that could be cool. :-)
Comment #56
glennpratt CreditAttribution: glennpratt commentedI was actually toying with making a module a .phar... but for now it's just a relative uri with no protocol. I definitely hoped we could handle that concept or phar much easier in the future by expecting a uri and refactoring a bit here.
Comment #57
Crell CreditAttribution: Crell commentedI don't mean a phar. I don't think a phar would work for non-code files very well.
What's the advantage of a full path then?
Comment #58
glennpratt CreditAttribution: glennpratt commentedIt's still relative. I changed the column to
uri
because that's the keydrupal_system_listing()
returns and it's mentioned in the comments around_system_rebuild_module_data()
. I'm not actually attached to it, if we want to change it topath
or something.Phar was just a thought, not something I actually want to address here - #1308152: Add stream wrappers to access extension files is way more interesting and I'll keep it in mind - though that wouldn't actually change what is in the system table.
I've got the patch working for a running install, but install fails and I haven't gotten to update yet.
Comment #59
peterx CreditAttribution: peterx commentedWe could replace a lot more than hooks in basic modules. The .info file could define all the static data used by the module including the schema.
Yaml might be the way to go if Drupal 8 is going to use Yaml or adopt framework code that uses Yaml. Something like Yaml gives us a way to add updates for existing data but Yaml is a pig and is the opposite of semantic. Updates and other major changes are best performed in PHP. The current .info format, while not ideal, is at least semantic and can cover most configuration requirements.
The .info file could easily be extended to replace the configuration settings usually put in the admin page. I wrote a module to do that for settings that do not require SQL or special code for validation. The code required was not significant.
Many schemas could go in .info files. If you are not specifying a rebuild of a table, the schema is a good target for replacement. .install files would only be needed for refactoring existing data.
I wrote a couple of extensions for .info that let you define schemas and related information in the .info. The code was relatively easy. Most of the same settings are shared between the schema, the settings form, views, and default views. Some of my modules have empty .module files as a result. Removing the .module requirement would be a useful next step.
Comment #60
Crell CreditAttribution: Crell commentedAt this time YAML is not planned for anywhere in Drupal. I would heartily suggest against trying to shove too much into the info file, lest it turn into a meta programming system of its own. There are other things we should do to the schema instead.
Comment #61
neclimdulSince its seems relevant and I'm tired of sitting on this gem waiting for it to come up:
Pop Quiz
Which of these work?
1) drupal_load('module', 'system');
2) drupal_load('theme', 'stark');
3) drupal_load('theme_engine', 'phptemplate');
4) all of the above
Which of these are documented to work?
1) drupal_load('module', 'system');
2) drupal_load('theme', 'stark');
3) drupal_load('theme_engine', 'phptemplate');
4) all of the above
Comment #62
glennpratt CreditAttribution: glennpratt commented1, 4? :)
Comment #63
glennpratt CreditAttribution: glennpratt commentedPosting a WiP patch for the bot, it includes #60 from #1299424: Allow one module per directory and move system tests to core/modules/system, which I depend on to move away from storing filenames in system (just directories now).
Tasks
Comment #64
glennpratt CreditAttribution: glennpratt commentedSorry, I forgot to include #60 from #1299424: Allow one module per directory and move system tests to core/modules/system as I claimed, well then the patch above shows my changes in isolation, which I realize could use some cleanup (like where temp. vars from using dirname($filename) may now be simplified.
Comment #66
glennpratt CreditAttribution: glennpratt commentedHuh, sorry for the spam... maybe it's the name.
Comment #68
glennpratt CreditAttribution: glennpratt commentedjthorson straightened me out - I accidentally let a couple Mac line endings in.
Comment #69
glennpratt CreditAttribution: glennpratt commentedComment #71
glennpratt CreditAttribution: glennpratt commentedFew fixes, still won't pass and still including #1299424: Allow one module per directory and move system tests to core/modules/system.
Comment #72
glennpratt CreditAttribution: glennpratt commentedComment #74
BassistJimmyJam CreditAttribution: BassistJimmyJam commentedRerolled patch now that #1299424: Allow one module per directory and move system tests to core/modules/system has been committed. This patch includes a few changes:
module_list()
(was appending the$module
array).simpletest_test_get_all()
andsimpletest_classloader_register()
.Comment #76
Mark TrappRe #58: at the risk of bikeshedding,
uri
isn't an accurate identifier. Per RFC 3986:And in Section 3:
If it's going to be a relative path,
path
is a more accurate identifier as URIs require a scheme component.Comment #77
glennpratt CreditAttribution: glennpratt commentedFrom the same RFC:
Comment #78
RobLoachSo we should rename
uri
topath
? Having it a file stream seems like a separate discussion. Even if it becomes a file stream, we still need to resolve it down the the modulepath
.Comment #79
xandeadx CreditAttribution: xandeadx commentedSimilar issue: #1058720: Lazy load modules
Comment #80
sunComment #81
ParisLiakos CreditAttribution: ParisLiakos commentedComment #82
David Strauss+1 for YAML! Another unnecessary Drupalism gone.
Comment #83
ParisLiakos CreditAttribution: ParisLiakos commentedi think this should be postponed on #1292284: Require 'type' key in .info.yml files
Comment #84
ParisLiakos CreditAttribution: ParisLiakos commented:)
Comment #85
ParisLiakos CreditAttribution: ParisLiakos commentedlets see with hal and serialization missing their module files
Comment #87
jrglasgow CreditAttribution: jrglasgow commented#85: drupal-340723.patch queued for re-testing.
Comment #89
ParisLiakos CreditAttribution: ParisLiakos commentedComment #90
ParisLiakos CreditAttribution: ParisLiakos commentedsigh, i am being overzealous:P
Comment #92
ParisLiakos CreditAttribution: ParisLiakos commentedhere is a slightly different approach which doesn't undermine performance:)
Comment #94
ParisLiakos CreditAttribution: ParisLiakos commentedhmm
Comment #95
ParisLiakos CreditAttribution: ParisLiakos commented...and without unrelated stuff..sorry for the noise
Comment #97
ParisLiakos CreditAttribution: ParisLiakos commentedthere, this should be green
Comment #99
ParisLiakos CreditAttribution: ParisLiakos commentedSo, lets improve update module in the process:)
i still need to figure out this weird exception in serialization module, but thats all thats left
Comment #101
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, turns out we have two serialization_test modules:)
Comment #103
ParisLiakos CreditAttribution: ParisLiakos commentedoh..
Comment #104
ParisLiakos CreditAttribution: ParisLiakos commentedthis issue definitely needs this tag:)
Comment #105
jibransites/all/modules/foo/foo.info.yml still works I think.
diff please
Comment #106
ParisLiakos CreditAttribution: ParisLiakos commentedhm? the binary thing? i thing dreditor reads it fine, but some editors dont..well, i had to extract this one, remove the empty .module file and recompress
Comment #107
ParisLiakos CreditAttribution: ParisLiakos commentedbundle_test.module does not exist anymore, manually edited the patch and reupload
Comment #108
catchfile_exists() on a file that doesn't exist doesn't get cached so this is potentially a bad performance hit - certainly it's going to be worse than just including an empty file as we occasionally do now. See the docs for http://www.php.net/manual/en/function.clearstatcache.php where this is explained.
We already scan for module location so it ought to be possible to record in the same place whether there's a file or not.
Comment #109
ParisLiakos CreditAttribution: ParisLiakos commentedactually what about that? it is consistent with what module handler does. This way the file_exists on this file will happen only once
Comment #110
catchThat's still an uncached file stat for each of these files for every request. If I can make the time I'll post some strace output.
Comment #112
ParisLiakos CreditAttribution: ParisLiakos commented#109: drupal-340723-109.patch queued for re-testing.
Comment #114
catchHere's the strace,
My 8.x install is busted atm (yes that's silly), so did this on 7.x:
Just include_once(), with a cold realpath cache.
With a warm realpath cache, notice the lstats disappear:
With file_exists() on an existing file (with a warm realpath cache),notice the extra 'access' up top:
file_exists() on a non-existing file whether there's a warm realpath cache or not:
Looking at this, I think the PHP docs might be lying when they say that file_exists() is cached by PHP, since the extra syscall happens either way. Will look at the source.
Cross-linking #752730: Remove file_exists() during bootstrap.
Comment #115
catchHere's what it looks like with apc.stat = 0:
apc.stat = 0 is successfully removing the stat call, but it doesn't do anything about the access either.
Comment #116
jibran#109: drupal-340723-109.patch queued for re-testing.
Comment #118
tim.plunkettReroll.
Comment #120
swentel CreditAttribution: swentel commentedreroll
Comment #122
XanoStraight re-roll from #120.
Comment #124
webchickSeems we should rename System Message to Tag Monster. :)
I'd still really love for this to happen. Right now we force module / theme developers to add an extra "type" line to their .info files (which, when it's inevitably missed results in several minutes of frustrating face smashing) in order to reap this benefit, and then didn't actually manage to reap it.
Comment #125
ParisLiakos CreditAttribution: ParisLiakos commentedsure..this should be green, but catch's concerns are not dealt with yet...dunno what i should do there? maybe unconditionally include the file, and just put the abomination symbol
@
in front of it...i dont think its an acceptable solutionComment #126
tim.plunkettI'd like to call this major, at least on the DX front.
With so much more code being OO, .module files are almost only used for hook implementations, which you might not even need.
Needing this file to exist becomes even more of a WTF, and looks really silly to a non-Drupal coder.
Thankfully, since including an empty .module file doesn't hurt anything, this does not need to go in before beta. But it sure would be nice.
Comment #127
dawehner125: drupal_340723_125.patch queued for re-testing.
Comment #129
star-szrGoing to try rerolling this.
Comment #130
star-szrRemoved the change to _system_rebuild_theme_data(), \Drupal\Core\Extension\ThemeHandler::rebuildThemeData() is using SystemListingInfo already and already looking for .info.yml for themes and .engine for theme engines.
Other than that there was just a minor conflict in simpletest.module.
Comment #132
star-szrAhem. This should install :)
By the way I didn't check to see if we need to remove any newly added empty .module files. I'm not sure that needs to happen in this issue, or we at least don't need to remove all of them here.
Comment #135
webchickDrupal.org--;
Comment #136
webchickLOL, $webchick--;
Comment #137
ParisLiakos CreditAttribution: ParisLiakos commentedshould be green again
Comment #139
ParisLiakos CreditAttribution: ParisLiakos commented137: drupal-340723-137.patch queued for re-testing.
Comment #140
webchickNice. :) That's a really impressive diffstat!
Comment #141
Crell CreditAttribution: Crell commentedIt seems like this is unnecessary now, if everything has an extension of info.yml, no? No sense keeping these as separate if/else blocks.
Everything else looked quite reasonable to me. Might this finally happen???
Comment #142
tstoecklerRe #141: The change to $dir is essential, so profiles do need their own code block. Alternatively we could do a switch-case and have 'profile' fall through to the rest after setting $dir.
Comment #143
catchThis is still a problem. Doing the check every request is a non-starter. The module file being removed is a PHP warning since we use include_once, and it's recoverable with a cache clear/rebuild anyway.
Feels like it ought to be possible to keep the file existence in either cached module handler, or with the module list that's already on the kernel. We even have some empty space to put that information.
Comment #144
ParisLiakos CreditAttribution: ParisLiakos commentedi think i have a solution for module handler.
But what about the file_exists in drupal_load()? or we dont care that much there?
Comment #145
catchDepends whether it's called during a normal page request. We should be serving pages (at least when internal caches are warm) without any file system checks by default. Where that's impossible, like PhpStorage, then it's swappable and can be replaced.
Comment #146
ParisLiakos CreditAttribution: ParisLiakos commentedhttps://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...
says it is only used in 2 tests and in two uninstall functions, so thats good news i guess..i ll roll a patch soon with module handler changes
Comment #147
catchThat is good news. We might want to mark it @deprecated even - big change from 7.x :)
Comment #148
ParisLiakos CreditAttribution: ParisLiakos commentedI am not sure we have an alternative way to load disabled modules. but anyway that should be discussed in another issue
Well, theme engines dont have .info.yml..but i changed it anyway. defaulting to
.info.yml
as extension makes senseUse that, and then it gets easy ;)
find . -type f -size -200c -name "*.module"
I got rid of the file_exists() call from module handler, but i think it seems more hacky now?
Perhaps we should make
container.modules
contain an array of arrays/objects instead of array of filenames like it is now, because everything else needs just the location of the module, module handler also needs the .module files.Well, lets see what breaks now.
Comment #150
catchSeems reasonable.
I opened #2159915: Remove drupal_load() for drupal_load().
Comment #151
sunI'm a bit concerned about the approach taken in the patch. It looks like we're adding at least one additional (expensive) file system scan, and we're hard-coding a range of assumptions that are internal to the system/extension discovery process into many different spots of the code base.
The root cause is that
drupal_load()
& Co previously received a.module
filename and simply included that — but now it gets an.info.yml
file, which is not a PHP file that can be included.Apparently, the theme system works around the identical issue already (
template.php
is optional, but needs to be loaded) in a similarly custom and scattered way, which explains why there are no theme related changes in this patch.I do not think that this is a clean and sustainable approach - instead of continuing the same poor pattern, we should change the extension discovery process to return the right information already.
For example:
That way, all of the consuming code no longer has to perform
str_replace('.info.yml', '.module', $file->uri)
(which is not its business to figure out), and we can also avoid additional filesystem scans for determining whether there are .module files that can be loaded.Thoughts?
Comment #152
ParisLiakos CreditAttribution: ParisLiakos commentedmaking SystemListing return those info in a single scan, would need larger refactoring and then fix everything else that breaks. i am not sure whether we should do it at this point..the extra scan only happens in the kernel anyway and i think this will be temporary since i doubt there will be .module files in D9?
Comment #153
sunI think it's essentially the same as @catch agreed to in #150 already. But instead of making the switch private to DrupalKernel only, we return the richer information from the system (info) listing already. Doing so ensures that everything else in the system has the same information available.
I was specifically pointing out that additional scan, because in reviewing various call chains very recently, I noticed that we're performing tons of full filesystem scans all over the place in D8 right now. As a direct consequence, it shouldn't be a surprise to anyone that e.g. tests are taking forever to run. Every single new filesystem scan is one scan too much.
Lastly, one of the original goals of #1292284: Require 'type' key in .info.yml files actually was to remove the concept of type-specific scans altogether. Instead, we can perform one and only one single filesystem scan for .info files to discover and prepare all of them in one go. That is, because reality is that whenever the system performs these scans, we're rebuilding all info and services, due to interdependencies between profiles, modules, and themes. → By consolidating all scans into a single spot of extension discovery, we can vastly simplify the process, improve performance, and eliminate many custom hacks that are scattered across the entire system currently.
Comment #154
ParisLiakos CreditAttribution: ParisLiakos commentedthanks, the last paragraph cleared things out for me.
it never occurred to me that getting rid of all the separate scans could outweigh the penalty of parsing all available info files
Comment #155
sunTo complete that train of thought, let me throw out a concrete proposal for how we can attack this:
Instead of creating anonymous objects on the lowest originating layer (SystemList), let's create proper, typed Extension objects.
As an additional bonus and benefit, make that Extension class extend SplFileInfo. → Killing a dozen of other epic issues in one go.
Attached interdiff is against the latest patch.
Comment #156
BerdirHaven't looked at it in detail, but it sounds related to what @fubhy is up to in #2024043: Add Module, Theme, Profile, and Extension value objects...
Comment #157
xjmSo this does not block beta, but @Dries said it should be considered a major. If we don't get this done by beta 1 then it needs to be postponed to D9.
Comment #158
catchDepends on exactly what changes are necessary to support it, but it the actual change itself is not API-breaking - so it could go in past beta or into 8.1.0 even - assuming we're not breaking APIs as a side effect.
Comment #159
webchickYeah, I agree that I think this can be done anytime. xjm might've gotten this one confused with a different issue. It'd be nice to have it done before beta though so that intro module dev tutorials don't have you creating a stupid blank module file, so +1 on "beta target."
Comment #160
sunI'm actively working on this, or more generally, the entire extension system problem space, in the two referenced issues.
I designed a new ExtensionDiscovery that fully replaces SystemListing due to infinite recursion and major performance reasons, which implements and resolves 50% of the problem space here by scanning for .info.yml files only. Instead of adding new filesystem scans, it removes plenty of them.
I think it makes sense to keep the remaining task of killing the requirement for .module (and thus inherently also .profile files, because installation profiles are internally treated as modules) for this issue here, after the new ExtensionDiscovery is in place. I'll re-roll/rebase this patch here against the new code then.
Comment #161
sunLet's see how far we get with this. :-)
Comment #163
sunFixed RegistryTest.
Comment #165
tstoecklerWow, patch looks incredibly awesome! Hope I have time for an in-depth review soon.
One thing struck out:
Is this even being called? hook_init() is dead, AFAIK.
Comment #166
BerdirWe still have a $engine_init callback for theme engines, apparently, so yes that is called. Shouldn't be documented as hook_init() but that's not related to this issue.
Comment #167
tstoecklerAhh, that's incredibly weird, but thanks a lot for digging that up @Berdir! :-)
Comment #168
swentel CreditAttribution: swentel commentedfwiw #2122087: Remove references to hook_init()
Comment #169
sunSince the vast majority of changes in the current patch are required cleanups for legacy code that does not use the new
Extension
class methods yet, and because (despite its size) the current patch does not even contain the actual removals of empty .profile and .module files yet, I decided to attack those necessary cleanups in a separate issue first:#2210197: Remove public access to Extension::$type, ::$name, ::$filename
Comment #170
sun#2210197: Remove public access to Extension::$type, ::$name, ::$filename has landed, so here we go :-)
Comment #171
sunNow on to the actual meat: Deleting all empty .module + .profile files.
@ParisLiakos's hint for leveraging
find
works great; I upped the limit to 220 bytes though (and verified that there are no false-positives); i.e.:Comment #172
tstoecklerThe patch looks great. In theory this should not yield any performance regression now, due to the improved extension discovery. Probably could still use some benchmarks, though.
One super minor nitpick:
This comment is no longer accurate.
Comment #175
tstoecklerWUT?!? I totally did not delete that patch file. I don't even have those permissions on d.o, so...
Comment #177
sun#2210197: Remove public access to Extension::$type, ::$name, ::$filename had to be reverted due to #2206501: Remove dependency on Drush from test reviews
Postponing until the situation is resolved. :-/
Comment #178
sunMerged 8.x.
Comment #180
sunAdjusted for temporary BC shim.
Comment #182
sunThis should come back green.
Comment #183
sunUpdated issue summary to reflect the proposed solution.
Benchmarks are not necessary, since this patch does not change any aspects of Drupal's bootstrap. The only difference is that
ModuleHandler
instantiates proper objects for each module (instead of dealing with an array of filenames), but those objects are trivial data objects.Comment #184
dawehnerIt is not really obvious why we handle profiles with a module handler. Can we at least leave a comment why profiles are controlled by the module handler?
So we do have install/uninstall and load, but now we add "addModule". First question: Why can't we use add() directly? Second question: Can we find a better name which makes it obvious that this does not enable/install the modules?
Comment #185
sunHm. I'm not sure how to address #184 in code, so replying for now:
Installation profiles have always been treated as modules, and registered and handled by
ModuleHandler
. That aspect is not introduced or changed by this patch.We might be able to separate the concerns more cleanly as part of #2186491: [meta] D8 Extension System: Discovery/Listing/Info, but to be honest, that has a very low priority on my task list, since ultimately, the profile happens to be handled identically to any other module (right now).
While I agree that this internal aspect of Drupal's architecture lacks documentation, I think this is the wrong issue to start adding it.
I intentionally opted for two discrete
addModule()
andaddProfile()
methods, in order to prevent any code from being able to callModuleHandler::add('theme', ...)
. That's what public APIs are for :-)While I agree that "add" is a sub-optimal terminology, we are running out of verbs in this space. An alternative might be "append", but that doesn't really make it any more clear that the specified module/profile is only added to the module list (but not installed, enabled, or loaded).
Since those methods are only called from special front-controllers in Drupal core (install.php, update.php, etc), I'd rather go with the current naming for now, and if necessary, consider to rename them in a follow-up issue.
Speaking of, those methods would be a wonderful case for using the
@internal
annotation (the opposite of public@api
methods), but alas, there's no agreement for introducing that annotation standard in core yet... so I don't want to run into that can of worms either here. ;)Comment #186
sunMerged 8.x + resolved conflicts after #1351352: Distribution installation profiles are no longer able to override the early installer screens
addProfile()
is actively used byinstall_begin_request()
now — #1351352 was the reason for why I prepared that method here already.I think we should move forward here.
Comment #187
BerdirAgreed, this looks great.
The performance problems of earlier patches no longer exist, because we check for the existence of the .module file during discovery, and at runtime, we just know if it exists. Means that adding/removing a module will need a rebuild, but there are worse things than that.
Comment #188
sunChange notice: https://drupal.org/node/2217931
Comment #189
catchYes this looks great, and it was really nice to come in here and not have to rant about file_exists() yet again ;)
Committed/pushed to 8.x, thanks!
Comment #190
alexpottHad to revert this since this broke run-tests.sh
Revert eba0b71 and pushed to 8.x.
The issue is caused by
in run-tests.sh
Comment #191
alexpottThis was caused by #1808220: Remove run-tests.sh dependency on existing/installed parent site going in.
Comment #192
sunThanks! I just wanted to post the exact same patch + interdiff literally at the same moment :-)
Comment #193
webchickTaaaaake 2!
Committed and pushed to 8.x. Thanks!
Comment #195
ExTexan CreditAttribution: ExTexan commentedThis is in D8.8. I recently copied one module into a new name and reworked it for a new feature. In that case, though, I didn't need the .module file, so I deleted it, as it was my understanding that the .module file is no longer required. Said deletion was done after the module had been enabled (not sure if that is important).
When I clear caches, I get this:
I don't get any errors when I use "drush cr" - but next time I clear caches in the site, I get the above error again.
Has there been a regression on this (non)requirement in D8.8 (or before)?