Problem

  1. The only procedural code that exists in Drupal 8 modules and installation profiles are hook implementations.
  2. Not every module/profile has a use-case for implementing hooks.
  3. But yet, every module/profile requires a .module/.profile file.

Goal

  1. Make .module/.profile files optional.

Proposed solution

  1. #2188661: Extension System, Part II: ExtensionDiscovery laid the ground work by discovering all extensions through their .info.yml files only.
  2. It also introduced a new Extension class, which takes three arguments:

    1. $type: The extension type; i.e., 'profile', 'module', etc.
    2. $pathname: The pathname of the extension's .info.yml file.
    3. $filename: The name of the main extension file; e.g., 'node.module'.
  3. → Simply make $filename optional.

  4. To make that possible, the module list used by ModuleHandler and DrupalKernel is replaced with a list of actual Extension objects; i.e.:

    <?php
    $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:

    <?php
    $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:

    <?php
    $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.

Files: 
CommentFileSizeAuthor
#191 340723.191.patch56.86 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,336 pass(es).
[ View ]
#191 186-191-interdiff.txt1.01 KBalexpott
#186 extension.optional.186.patch55.85 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,471 pass(es).
[ View ]
#182 interdiff.txt8.87 KBsun
#182 extension.optional.182.patch55.38 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,312 pass(es).
[ View ]
#180 interdiff.txt1.17 KBsun
#180 extension.optional.180.patch51.8 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,317 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#178 extension.optional.178.patch51.8 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#175 extension.optional.171.patch51.45 KBtstoeckler
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch extension.optional.171_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#171 interdiff.txt26.17 KBsun
#170 interdiff.txt8.88 KBsun
#170 extension.optional.170.patch25.89 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,122 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#163 interdiff.txt1.21 KBsun
#163 extension.uri_.163.patch69.7 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,471 pass(es), 29 fail(s), and 16,399 exception(s).
[ View ]
#161 extension.uri_.161.patch68.49 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#155 interdiff.txt2.39 KBsun
#148 interdiff.txt6.54 KBParisLiakos
#148 drupal-340723-148.patch49.05 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean.
[ View ]
#137 interdiff.txt1.17 KBParisLiakos
#137 drupal-340723-137.patch45.28 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 59,791 pass(es).
[ View ]
#132 interdiff.txt1.18 KBCottser
#132 340723-132.patch44.11 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 59,687 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#125 interdiff.txt838 bytesParisLiakos
#125 drupal_340723_125.patch43.99 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_340723_125.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

swentel’s picture

I 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.

Crell’s picture

That'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.

catch’s picture

Subscribing. 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?

beejeebus’s picture

@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.

catch’s picture

Justin: completely missed "this patch will just make the .info and .module dependency changes, and we can do the rest later." - of course!

chx’s picture

a small problem, though. themes have .info files as well, how will you make a difference? based on where they are? I guess that works.

Crell’s picture

Yep, the directory they're in is fine.

So who's going to roll a patch already??? :-)

beejeebus’s picture

i'm busy til the weekend, so i'll roll a patch then unless anyone else beats me to it.

swentel’s picture

Status:Active» Needs review

Ok, 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).

swentel’s picture

StatusFileSize
new14.62 KB
Failed: Invalid PHP syntax.
[ View ]

And now with patch

Status:Needs review» Needs work

The last submitted patch failed testing.

chx’s picture

Care to write up why you are doing work in drupal_load ? That strikes me as a bad approach performance wise.

axyjo’s picture

Subscribing.

swentel’s picture

@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.

aaron’s picture

I 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?

beejeebus’s picture

i won't be getting back to this for a few weeks, moving to another country is time consuming.

mfer’s picture

subscribe

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new14.75 KB
Failed: Failed to install HEAD.
[ View ]

Ok, 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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Berdir’s picture

StatusFileSize
new8.82 KB
Failed: Failed to apply patch.
[ View ]

Ok, 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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new14.93 KB
Failed: Failed to apply patch.
[ View ]

Patch imcomplete...

Dries’s picture

I'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.

Berdir’s picture

The 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().

Crell’s picture

Dries:

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.

Dries’s picture

Status:Needs review» Needs work

Thanks 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.

beejeebus’s picture

Issue tags:+Registry

tagging with Registry

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new12.36 KB
Failed: Failed to apply patch.
[ View ]

Ok, 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"

RobLoach’s picture

Status:Needs review» Needs work

Note 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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new10.42 KB
Failed: Failed to install HEAD.
[ View ]

- 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.

beejeebus’s picture

at the very least, i think we need some comments around this:

<?php
+  if ($filename && strpos($filename, '.info') === FALSE) {
?>

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.

beejeebus’s picture

Status:Needs review» Needs work

woops, that should be CNW.

Berdir’s picture

at the very least, i think we need some comments around this:

Agreed ;)

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.

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.

also, an upgrade path?

To do what exactly? The system table will be updated when system_get_module_data() is called.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new10.6 KB
Failed: 12087 passes, 1 fail, 0 exceptions
[ View ]

Added a comment...

beejeebus’s picture

about 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).

swentel’s picture

Why 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 ? :)

Berdir’s picture

Hm..

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...

Crell’s picture

With 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?

sun’s picture

subscribing

a small problem, though. themes have .info files as well, how will you make a difference? based on where they are? I guess that works.

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.

Crell’s picture

I 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.

Status:Needs review» Needs work

The last submitted patch failed testing.

lilou’s picture

Status:Needs work» Needs review

Setting to 'needs review' - testbot was broken.

Status:Needs review» Needs work

The last submitted patch failed testing.

Berdir’s picture

Status:Needs work» Needs review

Testbot, I don't believe you :)

Berdir’s picture

I'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.

beejeebus’s picture

berdir - 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.

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

I really fail to see why this feature is useful.

axyjo’s picture

@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).

David Strauss’s picture

This 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.

beejeebus’s picture

Assigned:beejeebus» Unassigned
beejeebus’s picture

Status:Needs work» Closed (won't fix)
Crell’s picture

Version:7.x-dev» 8.x-dev
Status:Closed (won't fix)» Needs work

This is not won't fix. At worst it's postponed. We can come back to it in Drupal 8.

peterx’s picture

I 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

glennpratt’s picture

Given 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

  • Only look for .info file, not .module or .profile

Load

  • Use file_exists() or is_file() at load time, cache hits. Similar to module_implements() cache. Adding a module file where one didn't exist will require cache clear.

System Schema

  • replace filename with uri
    • relative path to directory only
    • convention of .info, .module and .profile paths and presence are details that don't need to be in the system table.
  • Change PRIMARY KEY to uri
  • Do we need a UNIQUE KEY on (type, name)? Probably out of scope.

Pre-update hook

  • Presence of filepath / lack of uri in the results from system will build the properties as needed to be backwards compatible during upgrade.
Crell’s picture

When you say replacing filename with uri, do you mean something like #1308152: Add stream wrappers to access system files? Because that could be cool. :-)

glennpratt’s picture

I 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.

Crell’s picture

I 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?

glennpratt’s picture

It's still relative. I changed the column to uri because that's the key drupal_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 to path or something.

Phar was just a thought, not something I actually want to address here - #1308152: Add stream wrappers to access system 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.

peterx’s picture

The 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,

We 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.

Crell’s picture

At 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.

neclimdul’s picture

Since 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

glennpratt’s picture

1, 4? :)

glennpratt’s picture

Status:Needs work» Needs review
StatusFileSize
new20.77 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 340723-module-info-only-WiP-63.patch.
[ View ]

Posting 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

  1. Fix any broken tests.
  2. Create upgrade path.
  3. Test performance, cache if needed.
  4. Review code comments and documentation for changes.
glennpratt’s picture

StatusFileSize
new88 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 340723-module-info-only-WiP-64.patch.
[ View ]

Sorry, 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.

Status:Needs review» Needs work

The last submitted patch, 340723-module-info-only-WiP-64.patch, failed testing.

glennpratt’s picture

Status:Needs work» Needs review
StatusFileSize
new88 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in drupal-module-info-only-340723-66.patch.
[ View ]

Huh, sorry for the spam... maybe it's the name.

Status:Needs review» Needs work

The last submitted patch, drupal-module-info-only-340723-66.patch, failed testing.

glennpratt’s picture

StatusFileSize
new87.86 KB
FAILED: [[SimpleTest]]: [MySQL] 33,992 pass(es), 13 fail(s), and 72 exception(s).
[ View ]

jthorson straightened me out - I accidentally let a couple Mac line endings in.

glennpratt’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-module-info-only-340723-68.patch, failed testing.

glennpratt’s picture

StatusFileSize
new90.58 KB
FAILED: [[SimpleTest]]: [MySQL] 34,014 pass(es), 13 fail(s), and 9 exception(s).
[ View ]
glennpratt’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-module-info-only-340723-71.patch, failed testing.

BassistJimmyJam’s picture

Status:Needs work» Needs review
StatusFileSize
new24.82 KB
FAILED: [[SimpleTest]]: [MySQL] 34,979 pass(es), 13 fail(s), and 9 exception(s).
[ View ]

Rerolled 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:

  • Fixed extension being appended in module_list() (was appending the $module array).
  • Updated queries in simpletest_test_get_all() and simpletest_classloader_register().

Status:Needs review» Needs work

The last submitted patch, drupal-module-info-only-340723-74.patch, failed testing.

Mark Trapp’s picture

Re #58: at the risk of bikeshedding, uri isn't an accurate identifier. Per RFC 3986:

A URI is an identifier consisting of a sequence of characters matching the syntax rule named <URI> in Section 3.

And in Section 3:

The generic URI syntax consists of a hierarchical sequence of components referred to as the scheme, authority, path, query, and fragment.

URI = scheme ":" hier-part [ "?" query ] [ "#" fragment ]

hier-part = "//" authority path-abempty
/ path-absolute
/ path-rootless
/ path-empty

The scheme and path components are required, though the path may be empty (no characters).

If it's going to be a relative path, path is a more accurate identifier as URIs require a scheme component.

glennpratt’s picture

From the same RFC:

4.2. Relative Reference

A relative reference takes advantage of the hierarchical syntax
(Section 1.2.3) to express a URI reference relative to the name space
of another hierarchical URI.

relative-ref = relative-part [ "?" query ] [ "#" fragment ]

relative-part = "//" authority path-abempty
/ path-absolute
/ path-noscheme
/ path-empty

The URI referred to by a relative reference, also known as the target
URI, is obtained by applying the reference resolution algorithm of
Section 5.

A relative reference that begins with two slash characters is termed
a network-path reference; such references are rarely used. A
relative reference that begins with a single slash character is
termed an absolute-path reference. A relative reference that does
not begin with a slash character is termed a relative-path reference.

A path segment that contains a colon character (e.g., "this:that")
cannot be used as the first segment of a relative-path reference, as
it would be mistaken for a scheme name. Such a segment must be
preceded by a dot-segment (e.g., "./this:that") to make a relative-
path reference.

RobLoach’s picture

Title:make modules only require .info files» Make modules only require .info files

So we should rename uri to path? 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 module path.

xandeadx’s picture

sun’s picture

ParisLiakos’s picture

Title:Make modules only require .info files» Make modules only require .info.yml files
David Strauss’s picture

+1 for YAML! Another unnecessary Drupalism gone.

ParisLiakos’s picture

Status:Needs work» Postponed

i think this should be postponed on #1292284: Require 'type' key in .info.yml files

ParisLiakos’s picture

Status:Postponed» Active

:)

ParisLiakos’s picture

Status:Active» Needs review
StatusFileSize
new15.53 KB
FAILED: [[SimpleTest]]: [MySQL] 31,289 pass(es), 12,533 fail(s), and 7,420 exception(s).
[ View ]

lets see with hal and serialization missing their module files

Status:Needs review» Needs work
Issue tags:-Extension system

The last submitted patch, drupal-340723.patch, failed testing.

jrglasgow’s picture

Status:Needs work» Needs review

#85: drupal-340723.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Extension system

The last submitted patch, drupal-340723.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new37.46 KB
FAILED: [[SimpleTest]]: [MySQL] 57,809 pass(es), 16 fail(s), and 4 exception(s).
[ View ]
ParisLiakos’s picture

StatusFileSize
new36.3 KB
FAILED: [[SimpleTest]]: [MySQL] 59,136 pass(es), 14 fail(s), and 4 exception(s).
[ View ]

sigh, i am being overzealous:P

Status:Needs review» Needs work

The last submitted patch, drupal-340723-90.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new34.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-340723-92.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

here is a slightly different approach which doesn't undermine performance:)

Status:Needs review» Needs work

The last submitted patch, drupal-340723-92.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new48.76 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

hmm

ParisLiakos’s picture

StatusFileSize
new37.03 KB
FAILED: [[SimpleTest]]: [MySQL] 56,577 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

...and without unrelated stuff..sorry for the noise

Status:Needs review» Needs work

The last submitted patch, drupal-340723-95.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new3.3 KB
new40.17 KB
FAILED: [[SimpleTest]]: [MySQL] 57,971 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

there, this should be green

Status:Needs review» Needs work

The last submitted patch, drupal-340723-97.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new41.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,981 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

So, lets improve update module in the process:)
i still need to figure out this weird exception in serialization module, but thats all thats left

Status:Needs review» Needs work

The last submitted patch, drupal-340723-99.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new1.29 KB
new42.16 KB
FAILED: [[SimpleTest]]: [MySQL] 58,326 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

yeah, turns out we have two serialization_test modules:)

Status:Needs review» Needs work

The last submitted patch, drupal-340723-101.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new873 bytes
new42.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,139 pass(es).
[ View ]

oh..

ParisLiakos’s picture

this issue definitely needs this tag:)

jibran’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -721,9 +721,9 @@ function drupal_settings_initialize() {
+ * modules/foo/foo.info.yml

sites/all/modules/foo/foo.info.yml still works I think.

+++ /dev/nullundefined
index 478f038..615cfcc 100644
--- a/core/modules/update/tests/aaa_update_test.tar.gz

diff please

ParisLiakos’s picture

hm? 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

ParisLiakos’s picture

StatusFileSize
new41.83 KB
FAILED: [[SimpleTest]]: [MySQL] 56,377 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

bundle_test.module does not exist anymore, manually edited the patch and reupload

catch’s picture

Status:Needs review» Needs work
-    include_once DRUPAL_ROOT . '/' . $filename;
-    $files[$type][$name] = TRUE;
+    $file = DRUPAL_ROOT . '/' . str_replace('info.yml', $type, $filename);
+    if (file_exists($file)) {
+      include_once $file;
+      $files[$type][$name] = TRUE;

-    return TRUE;
+      return TRUE;
+    }

file_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.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new504 bytes
new41.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-340723-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

actually what about that? it is consistent with what module handler does. This way the file_exists on this file will happen only once

catch’s picture

That'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.

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -Extension system

The last submitted patch, drupal-340723-109.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review

#109: drupal-340723-109.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+DX (Developer Experience), +Extension system

The last submitted patch, drupal-340723-109.patch, failed testing.

catch’s picture

Here'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.

26378 lstat("/home/catch/www/7/modules/tracker/tracker.module", {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26378 lstat("/home/catch/www/7/modules/tracker", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
26378 lstat("/home/catch/www/7/modules", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
26378 open("/home/catch/www/7/modules/tracker/tracker.module", O_RDONLY) = 12
26378 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26378 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26378 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26378 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26378 mmap(NULL, 12685, PROT_READ, MAP_SHARED, 12, 0) = 0x7f31caf9e000
26378 stat("/home/catch/www/7/modules/tracker/tracker.module", {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0

With a warm realpath cache, notice the lstats disappear:

26606 open("/home/catch/www/7/modules/tracker/tracker.module", O_RDONLY) = 12
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 mmap(NULL, 12685, PROT_READ, MAP_SHARED, 12, 0) = 0x7fc6b917d000
26606 stat("/home/catch/www/7/modules/tracker/tracker.module", {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0

With file_exists() on an existing file (with a warm realpath cache),notice the extra 'access' up top:

26606 access("/home/catch/www/7/modules/tracker/tracker.module", F_OK) = 0
26606 open("/home/catch/www/7/modules/tracker/tracker.module", O_RDONLY) = 12
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
26606 mmap(NULL, 12685, PROT_READ, MAP_SHARED, 12, 0) = 0x7fc6b917d000
26606 stat("/home/catch/www/7/modules/tracker/tracker.module", {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0

file_exists() on a non-existing file whether there's a warm realpath cache or not:

26378 access("/home/catch/www/7/modules/foo/foo.module", F_OK) = -1 ENOENT (No such file or directory)

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.

catch’s picture

Here's what it looks like with apc.stat = 0:

87 munmap(0x7f9bd9806000, 117186)    = 0
27687 close(12)                         = 0
# just include_once
27687 open("/home/catch/www/7/modules/system/system.module", O_RDONLY) = 12
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=140088, ...}) = 0
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=140088, ...}) = 0
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=140088, ...}) = 0
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=140088, ...}) = 0
27687 mmap(NULL, 140088, PROT_READ, MAP_SHARED, 12, 0) = 0x7f9bd9800000
27687 munmap(0x7f9bd9800000, 140088)    = 0
27687 close(12)                         = 0
#file_exists() then include_once()
27687 access("/home/catch/www/7/modules/tracker/tracker.module", F_OK) = 0
27687 open("/home/catch/www/7/modules/tracker/tracker.module", O_RDONLY) = 12
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
27687 fstat(12, {st_mode=S_IFREG|0664, st_size=12685, ...}) = 0
27687 mmap(NULL, 12685, PROT_READ, MAP_SHARED, 12, 0) = 0x7f9bd981f000
27687 munmap(0x7f9bd981f000, 12685)     = 0
27687 close(12)                         = 0
#file_exists on non-existing file
27687 access("/home/catch/www/7/modules/foo/foo.module", F_OK) = -1 ENOENT (No such file or directory)
27687 chdir("/")                        = 0
27687 umask(022)                        = 022
27687 open("/dev/urandom", O_RDONLY)    = 12

apc.stat = 0 is successfully removing the stat call, but it doesn't do anything about the access either.

jibran’s picture

Status:Needs work» Needs review
Issue tags:-DX (Developer Experience), -Extension system

#109: drupal-340723-109.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+DX (Developer Experience), +Extension system

The last submitted patch, drupal-340723-109.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new42.42 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, optional-module-340723-118.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new43.03 KB
FAILED: [[SimpleTest]]: [MySQL] 58,861 pass(es), 9 fail(s), and 1 exception(s).
[ View ]

reroll

Status:Needs review» Needs work

The last submitted patch, optional-module-340723-120.patch, failed testing.

Xano’s picture

Status:Needs work» Needs review
StatusFileSize
new42.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,290 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Straight re-roll from #120.

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -Extension system

The last submitted patch, drupal_340723_122.patch, failed testing.

webchick’s picture

Seems 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.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new43.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_340723_125.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new838 bytes

sure..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 solution

tim.plunkett’s picture

Priority:Normal» Major
Issue tags:+Needs reroll

I'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.

dawehner’s picture

125: drupal_340723_125.patch queued for re-testing.

The last submitted patch, 125: drupal_340723_125.patch, failed testing.

Cottser’s picture

Assigned:Unassigned» Cottser
Status:Needs review» Needs work

Going to try rerolling this.

Cottser’s picture

Assigned:Cottser» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new42.93 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Removed 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.

Status:Needs review» Needs work

The last submitted patch, 130: 340723-130.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new44.11 KB
FAILED: [[SimpleTest]]: [MySQL] 59,687 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new1.18 KB

Ahem. 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.

The last submitted patch, 132: 340723-132.patch, failed testing.

The last submitted patch, 132: 340723-132.patch, failed testing.

webchick’s picture

Drupal.org--;

webchick’s picture

Status:Needs review» Needs work

LOL, $webchick--;

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new45.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,791 pass(es).
[ View ]
new1.17 KB

should be green again

The last submitted patch, 137: drupal-340723-137.patch, failed testing.

ParisLiakos’s picture

137: drupal-340723-137.patch queued for re-testing.

webchick’s picture

74 files changed, 75 insertions, 236 deletions.

Nice. :) That's a really impressive diffstat!

Crell’s picture

+++ b/core/includes/bootstrap.inc
@@ -780,14 +780,14 @@ function drupal_get_filename($type, $name, $filename = NULL) {
       // Profiles are converted into modules in system_rebuild_module_data().
       // @todo Remove false-exposure of profiles as modules.
       elseif ($original_type == 'profile') {
         $dir = 'profiles';
-        $extension = 'profile';
+        $extension = 'info.yml';
+      }
+      elseif ($type == 'theme' || $type == 'module') {
+        $extension = 'info.yml';
       }
       else {

It 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???

tstoeckler’s picture

Re #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.

catch’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -88,8 +88,11 @@ public function load($name) {
+      if (file_exists($filename)) {

This 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.

  /**
   * Holds the list of enabled modules.
   *
   * @var array
   *   An associative array whose keys are module names and whose values are
   *   ignored.
   */
  protected $moduleList;
ParisLiakos’s picture

i think i have a solution for module handler.
But what about the file_exists in drupal_load()? or we dont care that much there?

catch’s picture

Depends 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.

ParisLiakos’s picture

https://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

catch’s picture

That is good news. We might want to mark it @deprecated even - big change from 7.x :)

ParisLiakos’s picture

StatusFileSize
new49.05 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean.
[ View ]
new6.54 KB

We might want to mark it @deprecated even

I am not sure we have an alternative way to load disabled modules. but anyway that should be discussed in another issue

It seems like this is unnecessary now, if everything has an extension of info.yml, no? No sense keeping these as separate if/else blocks.

Well, theme engines dont have .info.yml..but i changed it anyway. defaulting to .info.yml as extension makes sense

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.

Use 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.

The last submitted patch, 148: drupal-340723-148.patch, failed testing.

catch’s picture

container.modules contain an array of arrays/objects instead of array of filenames like it is now

Seems reasonable.

I opened #2159915: Remove drupal_load() for drupal_load().

sun’s picture

I'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:

<?php
array(
 
'uri' => 'core/modules/node/node.info.yml',
 
'load' => 'core/modules/node/node.module', // can be NULL
)
array(
 
'uri' => 'core/themes/bartik/bartik.info.yml',
 
'load' => 'core/themes/bartik/bartik.theme', // can be NULL
)
array(
 
'uri' => 'core/profiles/standard/standard.info.yml',
 
'load' => 'core/profiles/standard/standard.profile', // can be NULL
)
?>

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?

ParisLiakos’s picture

making 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?

sun’s picture

I 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.

ParisLiakos’s picture

thanks, 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

sun’s picture

StatusFileSize
new2.39 KB

To 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.

Berdir’s picture

Haven't looked at it in detail, but it sounds related to what @fubhy is up to in #2024043: Add ModuleInterface, ThemeInterface, ProfileInterface, ExtensionInterface and corresponding classes...

xjm’s picture

Component:base system» extension system
Issue tags:-Extension system+beta target

So 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.

catch’s picture

Depends 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.

webchick’s picture

Yeah, 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."

sun’s picture

Title:Make modules only require .info.yml files» Make modules and installation profiles only require .info.yml files
Assigned:Unassigned» sun
Status:Needs review» Needs work
Issue tags:+API clean-up
Parent issue:» #2186491: [meta] D8 Extension System: Discovery/Listing/Info
Related issues:+#2185559: Extension System, Part I: SystemListing clean-up + performance; remove SystemListingInfo + drupal_system_listing()

I'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.

sun’s picture

Status:Needs work» Needs review
Related issues:+#2188661: Extension System, Part II: ExtensionDiscovery, +#2187717: Remove code duplication in system_list() and ThemeHandler::rebuildThemeData()
StatusFileSize
new68.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Let's see how far we get with this. :-)

Status:Needs review» Needs work

The last submitted patch, 161: extension.uri_.161.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new69.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,471 pass(es), 29 fail(s), and 16,399 exception(s).
[ View ]
new1.21 KB

Fixed RegistryTest.

Status:Needs review» Needs work

The last submitted patch, 163: extension.uri_.163.patch, failed testing.

tstoeckler’s picture

Wow, patch looks incredibly awesome! Hope I have time for an in-depth review soon.

One thing struck out:

+++ b/core/themes/engines/twig/twig.engine
@@ -25,11 +27,8 @@ function twig_extension() {
/**
  * Implements hook_init().
  */
...
+function twig_init(Extension $theme) {
+  $theme->load();
}

Is this even being called? hook_init() is dead, AFAIK.

Berdir’s picture

We 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.

tstoeckler’s picture

Ahh, that's incredibly weird, but thanks a lot for digging that up @Berdir! :-)

swentel’s picture

sun’s picture

Status:Needs work» Postponed

Since 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

sun’s picture

Status:Postponed» Needs review
StatusFileSize
new25.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,122 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new8.88 KB

#2210197: Remove public access to Extension::$type, ::$name, ::$filename has landed, so here we go :-)

  1. Rebased against 8.x.
  2. Consistency: drupal_get_filename() + 'system.$type.list' should always return info file pathnames.
  3. Removed completely unnecessary JsonSerializable workaround.
sun’s picture

StatusFileSize
new51.45 KB
new26.17 KB

Now 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.:

find core -type f -size -220c -name "*.module" -delete
find core -type f -size -220c -name "*.profile" -delete
  1. Removed all empty .module files.
  2. Removed all empty .profile files.
  3. Fixed install_load_profile() still assumes a .profile file to exist.
tstoeckler’s picture

Issue tags:+Needs benchmarks

The 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:

+++ b/core/includes/install.core.inc
@@ -492,7 +492,7 @@ function install_begin_request(&$install_state) {
     // Override the module list with a minimal set of modules.

This comment is no longer accurate.

The last submitted patch, 171: extension.optional.171.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 170: extension.optional.170.patch, failed testing.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new51.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch extension.optional.171_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

WUT?!? I totally did not delete that patch file. I don't even have those permissions on d.o, so...

Status:Needs review» Needs work

The last submitted patch, 175: extension.optional.171.patch, failed testing.

sun’s picture

Status:Needs work» Postponed
sun’s picture

Status:Postponed» Needs review
StatusFileSize
new51.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Merged 8.x.

Status:Needs review» Needs work

The last submitted patch, 178: extension.optional.178.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new51.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,317 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.17 KB

Adjusted for temporary BC shim.

Status:Needs review» Needs work

The last submitted patch, 180: extension.optional.180.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new55.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,312 pass(es).
[ View ]
new8.87 KB
  1. Fixed DrupalKernelTest.
  2. Renamed ModuleHandler::add() into addModule()/addProfile() + added docs.

This should come back green.

sun’s picture

Issue summary:View changes
Issue tags:-Needs benchmarks

Updated 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.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php
    +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php

    +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -141,6 +141,37 @@ public function setModuleList(array $module_list = array()) {
    +    $this->add('profile', $name, $path);
    +  }

    +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
    @@ -49,24 +49,44 @@ public function isLoaded();
    +  /**
    +   * Adds an installation profile to the list of currently active modules.
    +   *
    +   * @param string $name
    +   *   The profile name; e.g., 'standard'.
    +   * @param string $path
    +   *   The profile path; e.g., 'core/profiles/standard'.
    +   */
    +  public function addProfile($name, $path);

    It 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?

  2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
    @@ -49,24 +49,44 @@ public function isLoaded();
       /**
    +   * Adds a module to the list of currently active modules.
    +   *
    +   * @param string $name
    +   *   The module name; e.g., 'node'.
    +   * @param string $path
    +   *   The module path; e.g., 'core/modules/node'.
    +   */
    +  public function addModule($name, $path);

    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?

sun’s picture

Hm. I'm not sure how to address #184 in code, so replying for now:

  1. 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.

  2. I intentionally opted for two discrete addModule() and addProfile() methods, in order to prevent any code from being able to call ModuleHandler::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. ;)

sun’s picture

StatusFileSize
new55.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,471 pass(es).
[ View ]

Merged 8.x + resolved conflicts after #1351352: Distribution installation profiles are no longer able to override the early installer screens

addProfile() is actively used by install_begin_request() now — #1351352 was the reason for why I prepared that method here already.

I think we should move forward here.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Agreed, 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.

sun’s picture

catch’s picture

Status:Reviewed & tested by the community» Fixed

Yes 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!

alexpott’s picture

Status:Fixed» Needs work

Had to revert this since this broke run-tests.sh

[04:58:39] Command [/usr/bin/php ./core/scripts/run-tests.sh --concurrency 8 --php /usr/bin/php --url 'http://drupalorgqa.ctidigital.com/checkout' --keep-results --all --clean 2>&1] failed
  Duration 0 seconds
  Directory [/var/lib/drupaltestbot/sites/default/files/checkout]
  Status [255]
Output: [PHP Fatal error:  Call to a member function load() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 92

Revert eba0b71 and pushed to 8.x.

The issue is caused by

  $module_handler = $container->get('module_handler');
  // @todo Remove System module. Only needed because \Drupal\Core\Datetime\Date
  //   has a (needless) dependency on the 'date_format' entity, so calls to
  //   format_date()/format_interval() cause a plugin not found exception.
  $module_list['system'] = 'core/modules/system/system.module';
  $module_list['simpletest'] = 'core/modules/simpletest/simpletest.module';
  $module_handler->setModuleList($module_list);
  $module_handler->loadAll();

in run-tests.sh

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new56.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,336 pass(es).
[ View ]
sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks! I just wanted to post the exact same patch + interdiff literally at the same moment :-)

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Taaaaake 2!

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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