Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This patch was split off from New module administration page with many new features. It will do the following:
- Create .meta files to accompany each module file.
- Within the meta file, give each module a human readable name and a description.
- Take the module description out of hook_help for each module.
- In the module listing, parse the .meta files rather than loading the entire module file to get its description.
Benefits:
- 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.
- 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. ;)
Comment | File | Size | Author |
---|---|---|---|
#50 | info_files_0.patch | 41.69 KB | webchick |
#31 | info_files.patch | 41.29 KB | webchick |
#26 | meta_files_ini.patch_2.txt | 48.63 KB | neclimdul |
#25 | meta_files_ini.patch_1.txt | 49.29 KB | neclimdul |
#24 | meta_files_ini.patch_0.txt | 48.83 KB | neclimdul |
Comments
Comment #1
webchickWrong patch. ;)
Also, I went with Dries's suggestion to use the INI format.
Comment #2
webchickOk, 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.
Comment #3
Tobias Maier CreditAttribution: Tobias Maier commentedthere are no .meta files in your last patch...
Comment #4
webchickargh! >:\
k, I'll need to add those again later then.
Comment #5
webchickTry 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.
Comment #6
Dries CreditAttribution: Dries commentedTo discuss: should the extension be .meta or .ini?
Comment #7
Dries CreditAttribution: Dries commentedTo 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?
Comment #8
Dries CreditAttribution: Dries commentedReview:
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.
Comment #9
webchickre: naming convention, I think .meta is the best bet, for three reasons:
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.
Comment #10
chx CreditAttribution: chx commentedOnce 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
Comment #11
neclimdulBased on Dries input I removed the
module_get_meta_info()
. But instead of replacing it withparse_ini_file()
I reintroducedmodule_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()
.Comment #12
neclimdulMissed modules.inc when doing the diff. Rerolled.
Comment #13
webchickTested, 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?
Comment #14
webchick"you'd" refers to Dries/Drumm. :)
Comment #15
neclimdulOk, 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.
Comment #16
neclimdulUpon 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.
Comment #17
Dries CreditAttribution: Dries commentedchx: 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:
Comment #18
kika CreditAttribution: kika commentedwhat about having an extension as
modulename.info
? .meta sounds obscure to many, expecially for non-fluent-english-speakersComment #19
webchickDries: This will throw a parse error in parse_ini_file:
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.
Comment #20
Dries CreditAttribution: Dries commentedwebchick: 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.
Comment #21
webchickAh 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.
Comment #22
chx CreditAttribution: chx commentedFuck 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.
Comment #23
neclimdulOk, 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.
Comment #24
neclimdulthe 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.
Comment #25
neclimdulThis 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.
Comment #26
neclimdulFix double declaration of parse_meta function. Fix documentation. Pass only filename for flexibility and clarity. We are going to stick with this patch.
Comment #27
webchickOk, 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:
Therefore, this is consistent with the file format we're trying to emulate.
Comment #28
webchickThat should of course read exactly the opposite. :P Not sure where my head is today. :P
Comment #29
rstamm CreditAttribution: rstamm commentedMissing 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.
Comment #30
webchickD'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.
Comment #31
webchickOk, this one should be ready to go.
Comment #32
drummIs there a reason to use dirname() instead of drupal_get_path()?
Comment #33
scroogie CreditAttribution: scroogie commentedWell, 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.
Comment #34
kika CreditAttribution: kika commentedCan 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.
Comment #35
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #36
Dries CreditAttribution: Dries commentedWhile 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?
Comment #37
webchickDries, 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. :(
Comment #38
adrian CreditAttribution: adrian commentedThis 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
Comment #39
robertDouglass CreditAttribution: robertDouglass commentedwebchick, 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?
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedwe 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.
Comment #41
Owen Barton CreditAttribution: Owen Barton commentedI 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!
Comment #42
neclimdulMentioned 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.
Comment #43
scroogie CreditAttribution: scroogie commentedWhy 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.
Comment #44
drummThe 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?
Comment #45
Dries CreditAttribution: Dries commentedRobert 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.
Comment #46
robertDouglass CreditAttribution: robertDouglass commentedGlad to play the evil developer role ;-)
Comment #47
Steven CreditAttribution: Steven commentedFrom 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).
Comment #48
webchick@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"
Comment #49
drummThese 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.
Comment #50
webchickHere we go.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #52
neclimdulChecked over with a fine tooth comb. I think we've "covered all our bases" a couple times over. RTBC
Comment #53
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #54
webchickThis has been documented in the Updating modules section.
Comment #55
(not verified) CreditAttribution: commented