Allow one module per directory and move system tests to core/modules/system

If we are picky about still having the possibility of having more than one module in the same directory we might rename the config directory to {module_name}_config.

Personally, I think one module per directory is fine.

I would much rather have a consistent behavior for modules and have separate directories per module.

Ok, so, I don't actually have a problem with module per directory.

HOWEVER

1) I object to magic naming of directories. It would be very to allow the actual name of the directory to be configured, either through a .info property or a hook or something.

2) Moving a .module file around is problematic for users who simply untar modules over their existing modules. It leaves dead files around and can confuse the system, particularly if the file gets included twice. This is probably mostly covered here because the deeper directory should be identified, but *should be* and *will be* are separate things.

I would object to requiring module per directory so that we can magic-name a subdirectory.

Separate module per directory is the natural evolution of having all modules in a directory; until Drupal 5 or so, module files were just a .module file in /modules, and that was that. Also, someone still needs to get back to this issue: #340723: Make modules and installation profiles only require .info.yml files

So if that gets resolved, I've no problem with "a module is defined by a .info file, and whatever that location is, that's the module's folder. And only one module per folder."

I don't see any value to allowing some modules to name their config directory "config", some "configuration", some "defaults", etc. That just makes it harder to figure out what a given module is doing... especially if we want (and we do) to make the .module file itself optional. (See issue above.)

Module per directory is OK with me, as are magic names. I think there is a proposed standard for how to layout projects with multiple modules. The main module goes in the root dir or something. Maybe Dave Reid knows the URL?

Moving this to the core queue where it can get some more eyeballs and consensus.

I don't have a strong opinion about this yet.
I think it will make a few things easier: autoloading, unique config directory, etc.
On the other hand it will make for a deeper directory structure, which in some cases can hurt DX. But that's the price.

One thing i want to mention though:
If each module gets its own folder, then it would be tempting to no longer have the module name as part of filenames.
The result would be that I have tons of "something.txt" open in my file editor (which is currently gedit), with no idea which module that is.
The typical file editor shows the filename in each tab, but not the directory name. Let's keep this in mind.

One decision we also need to make is, can one module live in a subfolder of another module?
As I understand, with the current proposal, this would still be allowed.
The consequence: Given a directory name, you can not always identify which module that is.
Or can you?

parent-dir/parent.info
parent-dir/child/child.info
parent-dir/child/x.inc
parent-dir/misc/y.inc

Now x.inc would be considered as part of "child" module, while y.inc would be considered as part of "parent" module, because parent-dir/misc does not contain a *.info file.

Also, do we care how the sub-module subfolders are to be named and structured? Is there a risk of conflict with other "magic" folder names? (for hooks, autoload, etc)

I am very much for one module per directory.

The module directory name being magic, I'm somewhat indifferent about. If we're going to have PSR-0 autoloading in modules, there will be some magic directories in them, so I'm having trouble seeing the downside.

@donquixote - As for not having the module name in every single file, YES PLEASE. That level of redundancy has lead us to put meaning in the file extensions, rather than the name of a file (.info, .profile, .module). I would much prefer common extensions and having the meaning be in the filename, not the extension. For example, AndroidManifest.xml... in the Drupal pattern we would make that MySuperApp.AndroidManifest. Consider the context (path), and the reality is self describing and requires no special configuration of my editor. That kind of magic is worse to me then magic for locating files... that kind of magic means I'm dependent on documentation.

litwol did a grep through some of the modules he uses and it turns out some of contrib does this:

./menu_block/menu_block_export.module
./menu_block/menu_block.module
./views_datasource/views_rdf.module
./views_datasource/views_xml.module
./views_datasource/views_xhtml.module
./views_datasource/views_json.module
./relation/relation.module
./relation/relation_endpoint.module
./entity/entity.module
./entity/entity_token.module
./devel/devel.module
./devel/devel_node_access.module
./location/location.module
./location/location_node.module
./location/location_user.module
./date/date.module
./views/views.module
./views/views_ui.module

In Drupal core, core/modules/simpletest/tests is probably the biggest hit on this. Personally, I have no problems with it either way. Policy doesn't matter when you just want to get your stuff working.

[EDIT] litwol wanted to add that he used this command to find them:

find  . -maxdepth 2 -iname \*.module

That level of redundancy has lead us to put meaning in the file extensions, rather than the name of a file (.info, .profile, .module). I would much prefer common extensions and having the meaning be in the filename, not the extension.

That is not within the scope of this issue and thus not up for debate. Thanks.

Since the initial config patch landed, this is a critical issue now (as we can't release without adding and enforcing the policy or changing the system).

I'd like to see explicit approval from a few of the affected maintainers.

As one of the affected, I don't care much. We are on git now, adding a git mv to the D8 porting process won't hurt me.

I'm going to assume that there's something stopping the config files from not being merged (ie, using the file extension to identify the config file, like Views might have views.conf).

If it's simply a case of apathy on the file-based-config team versus apathy on the part of everyone else, I'd be in favor of going for the one that requires the minimum effort...

However, if it does go ahead with enforcing one module per directory, I'd like to see better documentation in bundling multiple modules into one download (ie Views + Views UI) because currently that's sort of a dark art (I don't even know if it's possible).

Anyway, i'll get back to the bikeshed now.

We shouldn't mess with things in the Drupal 7 contrib world. If we fix it in Drupal 8 Core, then contrib will follow the "standard". This doesn't really need to be a policy, as it actually means just cleaning up some of the file structure for tests and such.

1. This issue is about D8 only.
2. As of right now, D8's new configuration system enforces this change, as it's otherwise unable to dissect which configuration files belong to which module.
3. This policy, if in effect, only means that there cannot be two .info files in the same directory. A drupal.org project/repository/package may still contain multiple modules (e.g., typical API + UI modules, but also more). The modules merely need to be in separate directories.

I haven't seen anything more here about how we're going to handle the test modules inside simpletest/. Suggestions?

Edit: I guess it's just a matter of moving a lot of files and rolling a patch, eh? Do we need to use git am to keep the history of the files?

The following are the modules in Drupal 8 Core that have multiple .module files in the same directory:

./core/modules/update/tests/aaa_update_test.module
./core/modules/update/tests/update_test.module
./core/modules/update/tests/bbb_update_test.module
./core/modules/update/tests/ccc_update_test.module
./core/modules/node/tests/node_test.module
./core/modules/node/tests/node_test_exception.module
./core/modules/node/tests/node_access_test.module
./core/modules/simpletest/tests/theme_test.module
./core/modules/simpletest/tests/update_test_1.module
./core/modules/simpletest/tests/common_test.module
./core/modules/simpletest/tests/image_test.module
./core/modules/simpletest/tests/system_incompatible_core_version_dependencies_test.module
./core/modules/simpletest/tests/form_test.module
./core/modules/simpletest/tests/common_test_cron_helper.module
./core/modules/simpletest/tests/requirements2_test.module
./core/modules/simpletest/tests/update_test_3.module
./core/modules/simpletest/tests/file_test.module
./core/modules/simpletest/tests/requirements1_test.module
./core/modules/simpletest/tests/filter_test.module
./core/modules/simpletest/tests/path_test.module
./core/modules/simpletest/tests/system_incompatible_module_version_test.module
./core/modules/simpletest/tests/system_dependencies_test.module
./core/modules/simpletest/tests/taxonomy_test.module
./core/modules/simpletest/tests/batch_test.module
./core/modules/simpletest/tests/update_test_2.module
./core/modules/simpletest/tests/url_alter_test.module
./core/modules/simpletest/tests/system_incompatible_module_version_dependencies_test.module
./core/modules/simpletest/tests/database_test.module
./core/modules/simpletest/tests/session_test.module
./core/modules/simpletest/tests/system_incompatible_core_version_test.module
./core/modules/simpletest/tests/system_test.module
./core/modules/simpletest/tests/ajax_forms_test.module
./core/modules/simpletest/tests/module_test.module
./core/modules/simpletest/tests/menu_test.module
./core/modules/simpletest/tests/update_script_test.module
./core/modules/simpletest/tests/ajax_test.module
./core/modules/simpletest/tests/actions_loop_test.module
./core/modules/simpletest/tests/xmlrpc_test.module
./core/modules/simpletest/tests/error_test.module
./core/modules/search/tests/search_extra_type.module
./core/modules/search/tests/search_embedded_form.module
./core/modules/entity/tests/entity_cache_test_dependency.module
./core/modules/entity/tests/entity_test.module
./core/modules/entity/tests/entity_crud_hook_test.module
./core/modules/entity/tests/entity_cache_test.module

@xjm, stick the following into your ~/.gitconfig:

[diff]
  renames = copies

What if we just moved each into their own specific folwer?

./core/modules/entity/tests/entity_cache_test.module --> ./core/modules/entity/tests/entity_cache_test/entity_cache_test.module
./core/modules/simpletest/tests/menu_test.module --> ./core/modules/simpletest/tests/menu_test/menu_test.module

I think it's very sensible to restrict to one module per directory. It enforces a clear structure.

Simply, I have no objections.

As far as the modules in simpletest are concerned, those will have to be moved to sub-directories, too.

Ok everyone agrees. For this issue we need to do 2 more things.

1) create an issue that fixes the simpletest folder structure
2) add a new section on the coding standards

Something like this?

Folder structure
---------------------
In Drupal 8 and later each module must live in a seperate directory. The configuration system enforces this change, as it's otherwise unable to dissect which configuration files belong to which module. This only means that there cannot be two .info files in the same directory. A drupal.org project/repository/package may still contain multiple modules (e.g., typical API + UI modules, but also more). The modules merely need to be in separate directories.

So based on #9 it might be good to ping:

• JohnAlbin
• KarenS
• merlinofchaos
• Pasqualle or another maintainer for views_datasource
• fago
• bdragon

The restructure in #22 looks good to me.

Actually, #22 was scripted to account for future re-rolls, but I forgot to attach the script.

To be executed from within core/modules/simpletest/tests.

Doesn't account for the additional modules mentioned in #20.

Ah, looks like we'll also want to run sun's script from within node/tests, search/tests, update/tests, and entity/tests.

Edit: Oh, and +1 for getting them out of the simpletest directory.

merlin http://twitter.com/merlinofchaos/status/176412657463738370 agreed

Just so we're clear: submodules within subfolders would still be allowed?

Breaking that pattern would make me angry.

Unofficially on behalf of JohnAlbin menu_block won't be a problem at all.

agentrickard: Yes.

foo/foo.info
foo/config (for config files)
foo/lib (for classes)
foo/bar/bar.info
foo/bar/config (for config files)
foo/bar/lib (for classes)

All in one download from drupal.org/project/foo.

Totally fine.

And +1.

Generally I support this as well (now that #31 has been clarified) but I do have a couple questions:

What would the behavior be if someone did have two .module files in the directory? Which .module would be used? I'm guessing that if the main folder is /foo then foo/foo.module would "win" over foo/bar.module? If I'm correct about that, are we enforcing the convention that given a folder name of /foo, then the module *must* be named foo.module?

Assuming that the rest of the current logic is intact, then I suspect what will happen is something like:

find all $foo.info files
foreach ($foo.info file directory) {
  $modules['foo']['config'] = $foo_dir/config;
  $modules['foo']['lib'] = $foo_dir/lib;
}

(assuming someone fixes #340723: Make modules and installation profiles only require .info.yml files, please do so!)

Which in turn means that both modules in the one directory would "claim" config and lib and so forth, which in turn would cause all sorts of weird undefined behavior when those config files are loaded twice. Whether or not that would break anything in practice is, I suppose, dependant on what those files are doing. Let's not find out. :-)

Please note that my agreement is grudging and I really think we should make sure that the alternatives are explored before we do it. It would be sad if we remove this sorta feature without really thinking about whether or not we can do it a better way. As I said previously, I'm against more magic naming.

I'd be fine with a "one module per directory" rule - as I think it's going to group things better together and makes clear what file belongs to which module.

Updating modules which move around is currently quite a pain though - in particular if your module might be invoked before update.php does its work... I hope the new PSR-0 autoloader should be a help then.

If I'm correct about that, are we enforcing the convention that given a folder name of /foo, then the module *must* be named foo.module?

this is unacceptable, plain and simple. Plenty of countexamples in contrib.

yeah, #39 is not the proposal.

re #39/40: It should however still remain a best practice (unrelated to this issue).

agreed. best practice yes, hardwire, no.

Drush has a lot of black magic to:

* figure out what's the project for a given module
* obtain the installation path of a project
* fight wrong practices as adding the same project= line to separate projects' .info

The lack of policy in this area has been a continuous source of failures in drush package management (I figure out a similar problem is present in the update manager). So in what concerns drush, I don't mind best practices, but policies.

In concrete I'd like to see the main module in the root dir (per #5) or something like projectname.info (or projectname.project or whatever) in the root dir. Would be great if in addition such a file declares its extensions there explicitly, instead of using the project= key added by the d.o packaging system to each .info file it finds.

#22 + found some half-hardcoded paths.

@xjm: No, git am is not required. Git doesn't do anything special to store renames, it just works by being the same or a similar file.

Ok. Now moving node, search, update and entity tests around, too. Naively. Let's see what the results are.

wow, I wouldn't have thought that this would require so many file changes...

In light of that, I'm inclined to agree with @webchick... it would make sense to move the testing modules from simpletest/tests to system/tests/modules/$module, since we're already moving them here, and moving them to System is on the table since D7 already, so in the end, we'd move them twice for D8 unless we'd move them to the final destination now already. Merely not sure whether it's acceptable to only move the modules for now (not the .test files).

git.d.o down, so #45 might or might not be valid.

This would move modules and .test files to system module.

Retesting both:
#45: 1299424-test-directory-structure-45.patch queued for re-testing.

And:
#49: 1299424-test-directory-structure-49.patch queued for re-testing.

Let's see how this works.

Upgrade path tetsts passing locally now :)

Next round.

Those update tests are passing locally now, too.

#60 looks good to me. Installs and tests fine locally.

Does this policy also apply to themes? Seems like that would be a valuable consistency when we think about detecting projects and DX.

I don't know that you can put multiple themes in one directory now in the first place. I agree that we want to be consistent between modules and themes, though.

#60: 1299424-test-directory-structure-60.patch queued for re-testing.

#60: 1299424-test-directory-structure-60.patch queued for re-testing.

Note that this applies and tests well, but that new unmoved test cases have been added in the meantime. Reroll attached.

Given its fragility, should we consider committing #65, then following up with another patch to catch the new test cases? In the mean time we can start insisting that any new test cases/test modules follow the new approach once this goes in.

Sorry. More clear: #65 contains all new test cases (as of now), just not the patch requeued in #64.

Let's do this thing.

FYI: Commerce is a big one for sub-modules and I just looked and it already is putting modules in separate directories (aside from the UI module).

#65: 1299424-test-directory-structure-65.patch queued for re-testing.

Ah, yes: #1503184: Convert Graph.inc to PSR-0 (and make its documentation clearer). Thank you. Btw. you forgot to remove the file[] entry, there ;)

Oh ... and I forgot to remove the file[] = graph.test, too.

(Upload failed, doing wierd things to the issue. Retagging.)

Back to rtbc, time to commit!

#74: 1299424-test-directory-structure-73.patch queued for re-testing.

Committed/pushed to 8.x, this is less prescriptive than PSR-0 and much smaller in terms of directory changes given the surveys of contrib modules done above. Will need a change notice.

There are two separate but complementary changes in the patch: We moved all core modules into their own directories (which in practice mostly meant test modules), and we also moved base system tests out of the simpletest module directory into the system module directory. We need a second change notification for the second change.

Tagging novice for the change notification because this one is nice and straightforward. See http://drupal.org/node/1487226 for instructions. Please also update the issue summary to summarize both changes when you create the change notice. Thanks!

I can get to this later today.

Updated issue summary.

I think http://drupal.org/node/1540510 covers it. As this impacts those using system tests in their own testing, and devs writing in core only, it seems sufficient as is. If not, feel free to let me know, and I'll try again.

If all is OK, though, feel free to switch to RTBC or fixed as appropriate.

Excellent, that looks good. Thank you.

Thanks @Paul Simard! I added an example to the change notification and simplified the wording a little.

Updated issue summary.

Also updated the summary to more closely reflect the status of the issue.

(xjm) Update summary with current status.

Not that some tests also moved to their respective modules testing directory, not just to the system module.

(xjm) Grammar fix.

Regression:
This caused the database tests to go missing, see #1558268: Database tests are not available.