This patch was split off from New module administration page with many new features. It will do the following:

  1. Create .meta files to accompany each module file.
  2. Within the meta file, give each module a human readable name and a description.
  3. Take the module description out of hook_help for each module.
  4. In the module listing, parse the .meta files rather than loading the entire module file to get its description.

Benefits:

  1. Paves the way for more sophisticated information kept in module files, which could hold all sorts of benefits in the future, including telling what version of a module you're using, etc.
  2. Should dramatically decrease the number of people reporting White Screen of Death errors on the admin/modules screen, which anyone who does Drupal support on the forums or IRC will tell you is probably the most common problem. :)

Leaving "active" for now; will mark as "patch (code needs review)" when I get the module administration page parsing as described. I just don't want to lose all this work somehow. ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

FileSize
37.94 KB

Wrong patch. ;)

Also, I went with Dries's suggestion to use the INI format.

webchick’s picture

Status: Active » Needs review
FileSize
32.99 KB

Ok, here's the patch.

Talking about limitations of PHP's parse_ini_file function, I had to take out the ()s in the description for aggregator module; otherwise I got a parse error when attempting to read its meta file.

I've left it as-is for now, though I suspect parentheses may be just the tip of the iceberg of problems we'll encouter if we don't move to something more robuts like the custom parsing function in Merlin's original patch.

Tobias Maier’s picture

there are no .meta files in your last patch...

webchick’s picture

Status: Needs review » Needs work

argh! >:\

k, I'll need to add those again later then.

webchick’s picture

Status: Needs work » Needs review
FileSize
41.08 KB

Try this...

Warning: If you already have .meta files from the old patch, this will append to the meta files and probably cause all kinds of drama. ;) So I recommend testing this patch on a clean install.

Dries’s picture

To discuss: should the extension be .meta or .ini?

Dries’s picture

To discuss: if we load the module description from the .meta file, do we still have to store a copy of that description in the systems table?

Dries’s picture

Review:

  • I think we can eliminate module_get_meta_info(). These meta files ought to be a requirement, and they need to adhere to certain rules. No need for 'safe defaults' or extra loading cruft. Simply avoid loading the module when there is no .meta file and obtain the data with a simple $metadata = parse_ini_file($filename); call.

Thanks for splitting this in smaller patches, Angie. Rock on.

webchick’s picture

Status: Needs review » Needs work

re: naming convention, I think .meta is the best bet, for three reasons:

  1. It describes what the file is for -- providing meta information about modules.
  2. INI files are typically used in other software packages (PHP, etc.) for setting parameters and options and stuff (like max execution time, magic quotes on/off, etc.). These files aren't really doing that (at least not yet); they're all uniform in the way they describe modules.
  3. .ini somewhat locks us into using the INI format of "key = value" and, as noted, this has already run us into some trouble (you can't use parentheses in your module description anymore, for example, or you get a big ol' parse error). You also can't put stuff on multiple lines, which will probably become another problem in the forseeable future (how cool would it be to have admin help text here, for example, that documentation writers could easily edit without submitting patches? Translations of this info would just be a matter of creating plaintext meta files in the appropriate language -- no need for mucking with .po files).

My feeling is actually that we should be pro-active and just change the format now, rather than after the patch lands (if it does) or, heaven forbid, during the next release cycle, which I could see causing all kinds of problems and/or an even scarier regex. ;)

Good point about the description field in the system table. I'll try and work on that and the "don't load if there is no meta file" code later today.

chx’s picture

Once again (happiness for issues needlessly forced to split and no i want stfu on this) i need to talk against php_ini_file

The problem is that it has problems with the simplest characters -- see that how a () did not work. Also we know that in the past another PHP parser function changed in subtle ways (keeping / removing line endings). It's much better to rely on our own. Even if it's using regexps.

If the need for removing parenthesis is not enough then what is?

/me fumes

neclimdul’s picture

Status: Needs work » Needs review
FileSize
45.85 KB

Based on Dries input I removed the module_get_meta_info(). But instead of replacing it with parse_ini_file() I reintroduced module_parse_meta_file() function which uses the regex to parse the meta files. This returns the description flexibility we lost along with giving us room to grow. Also, there really isn't a reason not to. parse_ini_file() might have given us a speadup if we where loading this every page load but its an admin things used only when setting up drupal and when adding modules. Seems we should error on the side of flexibility rather than speed.

I took the "skipping modules that don't provide meta files" suggestion to heart as well. They are now skipped by module_rebuild_cache().

neclimdul’s picture

FileSize
49.01 KB

Missed modules.inc when doing the diff. Rerolled.

webchick’s picture

Status: Needs review » Needs work

Tested, works. Found a couple minor code consistency/"needs doc" issues that I pointed out to you on IRC, so temporarily marking back to "code needs work" until those are fixed.

The patch doesn't remove the description field from the system table, because that's still needed for themes. Moving themes to use the meta file system and reworking how those are loaded so this field can be removed seems like a good candidate for a follow-up patch to this one, unless you'd prefer these to both be handled at once?

webchick’s picture

unless you'd prefer these to both be handled at once?

"you'd" refers to Dries/Drumm. :)

neclimdul’s picture

Status: Needs work » Needs review
FileSize
49.8 KB

Ok, fixed coding standard issues. Added meta file information from #76549 to the PHPDoc for module_parse_meta_file() so we have a document of what's allowed and how the meta files work.

Will follow up in a little bit with the theme meta file version of the patch for review.

neclimdul’s picture

Upon review it would appear that removing the description field is relatively impossible without drastically changing the theme system. We would have to require all themes to make .meta files and the .meta files would have to be loaded with every page load to get the owner or we would have to cache it in some way.

However, as a follow up patch to this we could change the description field to owner and only use it for the theme system which should require only minimal changes to the theme related functions in system.module.

Dries’s picture

chx: rather than repeating the same thing over and over again, maybe you provide an example ini file that can't get parsed properly (due to a known bug)? This ini file (with ';'s) works fine for me:

name = "foo; bar .!:' baz"
description = "apple"
kika’s picture

what about having an extension as modulename.info? .meta sounds obscure to many, expecially for non-fluent-english-speakers

webchick’s picture

Dries: This will throw a parse error in parse_ini_file:

name = Aggregator
description = Aggregates syndicated content (RSS, RDF, and Atom feeds).

Just that. No "tricky" special characters or anything.

1. It's perfectly reasonable someone would want to put parentheses in their module descriptions.
2. When someone does accidentally put in one of these unexpected characters, it's not a matter of something benign like the the description just doesn't show up; you get a big, red, scary parse error.
3. The INI format severely restricts our ability to do more "advanced" stuff than merely key/value pairs. See the PHPdoc for the module_parse_meta_file() for details on what this allows.

Dries’s picture

webchick: all you need to do is put quotes around your string. It's actually a helpful error. It tells you something is wrong at line 2 (!) of your ini file.

webchick’s picture

Ah ok. Didn't know that. I can re-roll the patch with quotes around name/description.

What about kika's suggestion? Makes sense to me.

chx’s picture

Fuck that. I am sick and tired of this debate. Steven clearly outlined all the problems with parse_ini_file but let the "no regexp" camp win and let's use an inferior and error prone solution. Holding back such an important patch is no good over that.

neclimdul’s picture

FileSize
48.69 KB

Ok, here's a copy of the patch using drupal_parse_meta_file wrapping parse_ini_file which provides us the ability to build the filename, check that it exists and provide sane return values. It also provides a good place for documentation for what its worth.

Following shortly will be a patch without drupal_parse_meta_file for review.

neclimdul’s picture

FileSize
48.83 KB

the follow up patch without drupal_parse_meta_file(). Also I forgot to rename the .meta files to .info in the previous patch. They have been renamed with this patch.

neclimdul’s picture

FileSize
49.29 KB

This is the drupal_parse_meta_file as a wrapper for parse_ini_file patch with the meta files renamed from .info to .meta. Please review this and the previous patch. I've provided both do to the time crunch.

neclimdul’s picture

FileSize
48.63 KB

Fix double declaration of parse_meta function. Fix documentation. Pass only filename for flexibility and clarity. We are going to stick with this patch.

webchick’s picture

Title: Add .meta files to modules to store meta information » Add .info files to modules to store meta information
Status: Needs review » Reviewed & tested by the community

Ok, I'm setting this sucker to RTBC.

- Dries's concern about the INI format was addressed.
- The files have been renamed ".info" which should be instantly recognizable what they contain, for programmers and non-programmers alike.
- Represents a minor speed improvement loading the admin/settings/modules page (before patch, the page was generated in 316.25 ms; after it was 277.04 ms), and probably a major memory increase but my PHP doesn't have support for showing that, unfortunately. :(

The only "nit-picky" thing I could find is I wondered if all names and descriptions should be quoted, since Aggregator module's is. However, when I inspected php.ini, I see that they only use quotes when required:

docref_root = "/phpmanual/"
docref_ext = .html

Therefore, this is consistent with the file format we're trying to emulate.

webchick’s picture

...and probably a major memory increase...

That should of course read exactly the opposite. :P Not sure where my head is today. :P

rstamm’s picture

Missing CVS Id tag in .info files.

It's helpfull if the .info file includes additional information like project = drupal.
Then you can use this information for e.g. to group the modules in admin/settings/modules.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

D'oh @ $Id$ tags. You're absolutely right.

The project thing you're right on as well, but Dries wants the packages stuff in a separate patch.

webchick’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
41.29 KB

Ok, this one should be ready to go.

drumm’s picture

Status: Reviewed & tested by the community » Needs review

Is there a reason to use dirname() instead of drupal_get_path()?

scroogie’s picture

Well, it seems to me the call to drupal_get_filename() which is done in drupal_get_path() is not needed because the filename is the return value of system_listing, which in case uses file_scan_directory. Other than that, drupal_get_path() uses dirname() too.

kika’s picture

Can anybody explain, just in one sentence, why plaintext format was chosen over simple semantic xml format? I could imagine the answer is just it's simpler (and I almost buy that) but else? Note: I do not want a format war again, just interested in some common sense reasoning.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

@kika - yes, ini format is simpler for humans to read and write.

@drumm - drupal_get_path() calls drupal_get_filename() which does a bunch of processing and a query into the DB. Thats not needed since we know all that we need to know when we load these files.

tested and all works well. modules that were previously active and do not have .info files will continue to load as usual. i think thats reasonable behavior.

Dries’s picture

While this is an interesting patch that is starting to look good, it doesn't hurt to postpone it until the next release. Right now, the .info files aren't really that useful, and might just confuse people? Or am I mistaken?

webchick’s picture

Dries, this patch holds up:

1. Dependency system, to allow checking to ensure required modules are installed and enabled. A big usability and a developer win, as currently the only way to implement some pseudo dependency system is with a fairly complicated hook_form_alter on the system form.
2. Packages system, which will help us solve a long-time problem of sub-modules that belong to the same module being spread out all over the listing (e-commerce, CCK, etc.) -- fixing this would be huge, usability-wise.
3. The ability to store version numbers and compatibility information, to help people track down what they're running, another usability improvement, and one of our most frequently asked questions.
4. Almost infinite other possibilities, once this basic infrastructure's in place.

The reason this patch by itself doesn't do much, is becase we were specifically told not to cram a bunch of features into one, and instead put them in as "baby steps." We could easily add in the packages and dependency stuff to this patch to make it more palatable, but my goal was to get this in, since so much functionality is dependent on it, so we made it as simple as possible. :(

adrian’s picture

This patch, in my mind , holds the exact same value proposition as 'put modules into their own directories'
The patch itself was simple enough to do it, but it opened so many other possibilities for us.

There's many many many places meta information is incredibly useful, and until we have a standard place to put it all, it's not going to be
able to reach it's potential.

Furthermore, this fixes the memory bug on the admin/modules screen, that in itself should be seen as a critical bugfix, as it's an incredibly commonly occurring problem.

Additionally, the 'description' column in the system table is being used for templates to store their template engine, and quite frankly, we haven't
been displaying the description field out of the database for a while now, it's essentially been dead data.

+1

robertDouglass’s picture

webchick, this looks really interesting from a contrib module developer's point of view. Correct me if I'm wrong, but using this, I could provide a list of terms that could be edited by the site admin to provide the base strings for later translations, right? The example I have in mind is the buddylist module. I hate the term "Buddy" and see "Contact" or "Friend" as common replacements for "Buddy". So with the new metadata files, I could instruct the site admin to edit a list of original translations that would then set the base term for Buddy. Is this one type of use case that is envisioned?

eg:
Buddy = Friend // <--- site admins can edit this

But in the current patch, the only metadata is the modules' descriptions, and that is loaded once and then saved to the database. I would have expected something different, I guess. I would have expected that the results are loaded (and maybe cached) and made available as part of the variables array. Is that intended for later? Seems like something along those lines would be dependent on a better variables system.

So, to keep me on track, what exact benefits do we get by having just this patch (and none of the great features you mentioned that depend on it) in the current release?

moshe weitzman’s picture

we are trying to get these and get dependencies in. both patches are ready IMO. i do think these represent signifcant value. Also, I can think of a few developers who will be heartbroken with a response like 'looks good, but lets wait. not that important'. they worked hard to split this patch from the module rework patch.

i think contrib modules may find good uses for .info files in this release even if core does not.

Owen Barton’s picture

I did some profiling of this patch using xdebug. The following values are for the /admin/modules page on a default install with the default module selection. I refreshed this page 5 times before I started recording values (to avoid any cache shuffling).

CPU Without patch
* 7,203,918
* 7,206,157
* 7,272,276
CPU With patch
* 6,580,908
* 6,443,758
* 6,804,215

Memory
Without patch
* 60,607,872
* 60,607,872
* 60,607,872
With patch
* 29,908,176
* 29,908,176
* 29,908,176

As you can see there is an approximate 10% decrease in execution time/cpu usage with the patch, and that the memory usage has halved.

This is very significant in my opinion, as it makes it more likely people will be able to run Drupal on hard memory limited vservers without reverting to renaming .module files to .module.old. This - combined with the fact that this patch could reduce the 'white screen of death' problem - make this an important improvement!

neclimdul’s picture

Mentioned earlier is the dependency patch waiting on this. You can find that at #81631. I'm working on finishing up the packages patch which I hope to be finished shortly.

scroogie’s picture

Why postpone it to the next release? It was already nearly in when it was one big patch :(
The whole module page rework is really great, and the depedency system is something which is really missing imho.

drumm’s picture

Status: Reviewed & tested by the community » Needs review

The most common place I've heard of to get an out of memory error is the modules page, which is why this is good for this release.

Where does the t() happen?

Dries’s picture

Robert wrote:
webchick, this looks really interesting from a contrib module developer's point of view. Correct me if I'm wrong, but using this, I could provide a list of terms that could be edited by the site admin to provide the base strings for later translations, right? The example I have in mind is the buddylist module. I hate the term "Buddy" and see "Contact" or "Friend" as common replacements for "Buddy". So with the new metadata files, I could instruct the site admin to edit a list of original translations that would then set the base term for Buddy. Is this one type of use case that is envisioned?

That is exactly what we do not want to see happen, and what is one of my reservations with this patch. It will introduce an alternative mechanism to variable_get and people will abuse it.

robertDouglass’s picture

Glad to play the evil developer role ;-)

Steven’s picture

From what I can tell, it is impossible to use the quotes character " in ini file values. There is no escape mechanism as far as I can tell. This is pretty darn annoying.

Also, the fact that we can only parse actual files (instead of just string variables) could be annoying in the future. If we want to download module meta data from drupal.org, we would need to write it to disk first (e.g. to automatically see if there is a new drupal version out).

webchick’s picture

Status: Needs review » Needs work

@Robert: Just this patch by itself:

1. Solves the infamous "white screen of death" problem at the admin/settings/modules screen (critical bug fix)
2. Allows modules to have "friendly" names like "Blog API" rather than "blogapi" (minor usability improvement)
3. Provides a standardized way in core for modules to provide meta information about themselves, which could be built upon in future patches. (normal/critical feature, depending on where you draw the line)

New patch forthcoming which:

1. Places descriptions in t()
2. renaming meta to info in function names
3. Improved PHPDoc on the parsing function
4. Prefixing the function name with a _ so it is "private"

drumm’s picture

These files should /never/ be edited by an end user (just like the module file itself). Any hack like using it to set some strings for buddylist is quite bad.

webchick’s picture

Status: Needs work » Needs review
FileSize
41.69 KB

Here we go.

moshe weitzman’s picture

I will police the abusers with a baseball bat. lets not hold up the patch because we are afraid of misuse. the DB API can be misused, the filter API, etc.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Checked over with a fine tooth comb. I think we've "covered all our bases" a couple times over. RTBC

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

webchick’s picture

This has been documented in the Updating modules section.

Anonymous’s picture

Status: Fixed » Closed (fixed)