Remove the registry (for functions)

subscribe

subscribing

But -1 for removing the registry.

subscribe

Subscribing and very -1. The page split in D6 was measured and found to be a 20% performance boost and 25% memory reduction (when not using an opcode cache), on a Drupal version that was overall slower than its predecessor. I have yet to see anyone question those benchmarks. Rolling that back gives us what advantage?

This is not going to be absolutely on topic rather than just some thoughts.

I added node_rewrite_sql so many years ago. Just to show, this was the regexp originally
$query = preg_replace('/(SELECT.*)('.$nid_alias.'\.)?nid(.*FROM)/AUs', '\1'. $nid_to_select .'\3', $query);

That's not that bad. But it was broken and the fix brought us 4-5 years of fighting with DISTINCT. We (I?) probably should have done much smarter much earlier than trying to fix a horribly complex regexp.

We made two fundamental mistakes with the menu system. One, the ancient storage model we were using as recently as Drupal 5.0 was created for approx a hundred entries and that really did not bode well when Drupal sites exploded to having 1e6-1e7 magnitude pages. Two, the menu tasks were a great idea and the small problems with default tasks grew to a colossal !#$%^ when the seemingly small change of secondary tasks got introduced. To recap, when you are on user/1/edit and you navigate on to user/1/edit/foocategory then you to be able to navigate back so there will be two tabs, one being user/1/edit the other being the default category which point to the same page while storing this in a structure which is keyed by paths. This is only being fixed right now.

What of the registry. There are already and will be enough posts about the speeds and the gains and the other drawbacks, I am trying to give a higher level view. I wish I had such concrete problems as above -- I do not. In general, however, the registry works marvelous when your Drupal is booted up and repeatedly serves the same pages over and over -- which, let's face it, is what it does most of the time. However, when you install, update or test or very early in the bootstrap, we are facing problems from time to time. I am truly wondering whether we spend a lot of effort getting things working when there are not enough gains. From a recent issue it was clear that even Dries had absolutely no clue what the registry does or how it works. This is worrying too. It might have been a temporary lapse on his part but color me worried.

So, as the person who worked second most on the registry: +ε (never mess with a maths teacher :p ). I am not +1 but I am not -1 either rather I am very cautiously positive towards removing and I want to see a lot of numbers before I decide.

If we re-introduce front-loading of files, that would reduce the _registry_check_code() calls, I suspect. That way the code we'll need will be available already, so we don't need to lazy-load it.

The registry should alleviate the need to have drupal_theme_rebuild() parse all files. It can just scan through the registry table for functions named theme_* rather than trying to parse files and do function_exists() or whatever it does.

28 MB to 20 MB is a non-small savings. If that's a consistent percentage difference rather than a constant difference, that's huge.

If the improvement on a non-APC site is large enough and the penalty on an APC site is small enough, I still think it's worth it. We have to keep Drupal functional on shared hosts, and sites running APC are more likely to be able to make custom tweaks or throw hardware at a problem than shared hosts are. (Naturally "large enough" and "small enough" are highly subject terms subject to interpretation.) The hit on APC sites seems to be mostly on cache-rebuild type operations, though, not on normal page views, isn't it?

On admin/build/modules we do a full theme/modules/menu rebuild - on my system this equates to 55mb memory usage, 2.5s page execution time and about 700 queries.

Is this without contrib?

This one kill most of shared hosts or we should manually ini_set('memory_limit', 268435456); or something to handle core+contrib

Suppose here is a memory leak as I dig into registry.inc - there's no load of all files only parse but maybe loaded resources stay in memory?

As I pointed before - I prefer 2 packages. At this moment I think that $menu['file'] is more flexible way to join all module's parts in one LOB. But maybe I'm wrong.

As I see menu_rebuild() invoke module_implements('menu') so it loads all modules in memory and IMO deprecates registry parsing that should works without loading files in memory :(

my stats for D7 + all modules enables + devel (admin/build/modules)

eaccelerator enabled:
Memory used at: devel_boot()=0.27 MB, devel_shutdown()=6.67 MB, PHP peak usage=7.25 MB.

eaccelerator disabled:
Memory used at: devel_boot()=1.82 MB, devel_shutdown()=21.63 MB, PHP peak usage=22.25 MB.

with return; at menu_rebuild

eaccelerator enabled:
first run Memory used at: devel_boot()=1.64 MB, devel_shutdown()=18.57 MB, PHP peak usage=19.25 MB.
next run and others Memory used at: devel_boot()=0.26 MB, devel_shutdown()=2.86 MB, PHP peak usage=3.5 MB.

eaccelerator disabled:
Memory used at: devel_boot()=1.83 MB, devel_shutdown()=17.78 MB, PHP peak usage=18.25 MB.

Forget to say - this is a windows box (php 5.2.9-2 + apache 2.0.63)

The registry would help contrib a lot and I find that it actually helps the developer experience. For any hooks for a contrib module, you can stick them in mymodule.contribmodulename.inc, and the registry takes care of including the file when appropriate. It also seperates the code nicely so you know which hook is being called where. I'm sticking a big -1 for removing this. It's one of the great additions to Drupal 7 and I don't want to see it go away.

Subscribe.

I don't know if registry should go or stay but i recoil at shipping 2 versions of drupal or shipping with code that splits your drupal for you (or unsplits).

So, I see some positives and negatives of the registry.

Positives:

• Improved DX for separating code out. (see #22)
• Better for memory on shared hosts where a large part of our install base is.

Negatives:

• There is a hit to sites running APC.

A lot of drupal sites are running on shared hosts and we need to keep that up. It seems this is a place where we can't make everyone happy.

Would it be possible to make the registry system plugable so we can ship it with a system that's optimized for shared hosts and have a contrib module providing a system better for apc setups?

If we are seriously considering alternatives to the registry, I can only suggest we replace it with dumb class wrappers. Those wrappers could be loaded based on naming conventions, or a hook_autoload_info() as defined by the autoload module.

I think it doesn't make any sense to consider putting everything back in one file. The performance difference would be negligible, while it obviously harm DX and the readability of the code.

The time saved by loading fewer files is very OS dependent. On most modern OSes that gets cached anyway so it's not as big a difference as it used to be. (Which hosts are running a "modern OS", of course, is difficult to say.)

Even if we consider not leveraging the registry for functions, PHP's native support for class overloading makes still using it there a 100% no-brainer to me. Bringing back front-loading of files, which the registry had originally, can then avoid nearly all autoload triggers.

I am still -1 on pseudo-classes. Rather, there are plenty of places where we could/should use a class anyway for its own reasons, and the cleaner autoloading is a nice bonus.

I really feel it important to reiterate that even with Drupal 6 *not* loading every file in a module on every page load (page callback files, most views code, etc.) it's already a horrific memory hog. "Load it all and be done with it" is simply not an option on a non-opcode server, and probably not even on an opcode-cache-using server.

I honestly do not know the memory architecture of APC well enough to say; just that I've heard mixed things about how it allocates and reports memory between processes, which is enough for me to say that there may be an impact, or may not. I do not actually know. If the actual answer is that "memory-wise APC is fine no matter what we do", I'm cool with that. For a non-APC site, though, our memory issues are critical.

Front loading was removed in the module_implements() refactoring patch that chx and, I think, justinrandall were working on a few months ago. I don't have an issue number off hand. Instead the registry caches what files to include for each hook implementation and just loads those, then iterates just those function names rather than every $module_$hook possibility. While an improvement, I think it is actually bad to remove the front loading because we have a lot of indirect code that is not hooks.

catch, that webgrind screen is with the patch in #32 applied, which rolls back the module_implements() caching? Sounds like having that is still good, but that doesn't mean the front caching wouldn't be bad. They're not mutually exclusive, and the module_implements() caching is only applicable for hooks. It doesn't help with callbacks, classes, or theme functions.

The front caching was part of the original registry patch, committed on 6 May 2008. It wasn't a separate issue.

If you are gunning to try and reimplement it, it would involve recording every file that gets included in _registry_check_code() and then on exit recording that list to the cache table, keyed by the menu router item. If it hasn't changed, do nothing. If it has, build up an ever-increasing list of files to include. The net result is that we probably end up including a bit more code than we need on a given page request, but after the first request or three we almost never have to include anything after the initial load.

To get a proper test out of it, you'll need lots of theme functions, callback functions, and classes, since hook implementations are now handled by module_implements(). (I fear what will happen without front caching when we add Views to the mix, with its complex class hierarchy.) Exactly how those two will interact I am not entirely sure. chx or justinrandell could say better.

Very much in support of this. Will review this week.

I had been working on a patch to flag certain files as pre-cacheable and some not, to avoid the issue mentioned in #40. When the alternate caching went in and the front-caching was ripped out, I won't fixed it as it did not apply anymore at the time. I still think that's a useful approach, though, at minimum for class autoloads as those will never be caught by the module_implements() cache.

Subscribe

my thoughts:

remove the registry except for the bit that auto-loads classes/interfaces. Probably can use a simple regex instead of tokenizing.

consider whether there are benefits to retaining a registry (built on the fly?) of which modules implement which hooks.

In e-mail Jacob suggests we shoudl jst rm the auto-loading too for simplicity. I think removing auto-loading would require some reworking of
simpletest. Otherwise I think it's minor either way. We are currently trying to use drupal variable to hold a class name in some
factory methods. They can always be key => value with class and filename.

There is no simplicity to be gained by removing the class autoloading. If anything, there's complexity to be added because then we have to manually track not only class location but class hierarchy. That is, we need to do drupal_class_exists() and such like we do now for functions, only it will be uglier because we have to deal with class parentage. Views is forced to do that now, and it's quite ugly and prone to fatal errors if you're not careful. If anything, we need to bring back the front-loading of classes to make it faster/cleaner, not remove it.

Unless of course you want to just load all of Drupal's code on every single page load. Class autoloading is a PHP language feature intended to solve this exact problem (performance and memory). To not make use of it is quite simply short-sighted and naive.

Really, I put this out to anyone suggesting we remove the registry, rather than try to further optimize it: Find me another approach that gives us at least a 10% improvement in performance and manages to keep Drupal's memory usage low enough that Drupal doesn't mandate the use of an opcode cache for a reasonably complex site. In the past year, no one has come forward with a way to address our rather drastic memory issues other than "load less code". If you have a better idea, speak now or forever hold your peace.

And "just use an opcode cache" is not an answer.

This is part of an email I sent to an Acquia list describing what a scenario which happened yesterday:

The only way to make the current registry smart enough to not bail on normal developer behavior is to trap all require errors and undefined function errors and try to reload / compensate as they happen.

Another example of registry naiveté we found the other day:

We had a class called Palette which used to be a standalone class. It was in palette.inc which was referenced in themebuilder_compiler.info like this:

file[] = palette.inc

Okay, that's kinda handy, because we never have to include it, even if a bit opaque, but then, Katherine made a check in:

A inc/colorset.inc
M palette.inc
M themebuilder_compiler.info

She changed the signature of
- class Palette {
+ class Palette extends Colorset

and in themebuilder_compiler.info
+ file[] = inc/colorset.inc

Okay, so this works great with the registry, and palette.inc does not have to include inc/colorset.inc. However, it means that until the registry gets rebuilt, I get a fatal error on every page as palette.inc is included on every page, and the class it is extending does not exist in the included files.

So I can't even go to admin/structure/modules to reset it. For anyone not interested in editing the registry table in MySQL, this means a re-install.

So the only way to solve this is:

a). Build a recovery script which stays outside of the registry which can reload it in some intelligent way.
b). Change the error handler to catch these errors, know every possible include related error and rebuild the registry, and then try again. This is also problematic because a handling the error will screw up the page flow requiring us to essentially resubmit the request which can cause data integrity issues.

The problem of loading too much code is endemic in plugin based architectures. I don't see how it can be avoided because plugins need to be available to respond to events. The only option would be voluntary disclosure of which events(hooks) the plugin wants to implement, and run-time inclusion. But even that has performance implications AFAIK. It's also a pain the ass. What I mean is, in the info file: hooks[] = nodeapi;

The clear path forward I think is backwards.

I'm also against the autoloading facility. I know it makes some things simpler and I like it for performance reasons, but at the cost of complexity for incoming developers, and this is NOT worth it. Drupal is already a pain to learn because there are so many anonymous array structures not connected to error handling and control based on naming schemes. If I use type instead of #type, I get some bizzare error 5000 lines of code later, etc..

We need less mystery meat APIs, not more behind the scenes voodoo, and until it is very clear how it finds the files to include, and it actually works without hosing the site, I think it hurts more than helps.
----

To respond to Crell's last point about an autoloader being there for this purpose. I agree, and I also think it should only be used for classes, and I think classes should maintain namespacing with directories and files (ala PEAR) to make this simpler. For functions, it's just not practical IMO.

Don't get me wrong, I'm all for the cause of making Drupal faster and more accessible to small hosts. But I think what we've got now is untenable for module development and has no obvious path to viability.

If someone can answer Crell's challenge, I think it would be great. I for one don't claim to.

-J

It would be naive at this point to think we could simply rollback the registry, especially without drafting a way forward first. The registry does two things:

1. lazy-loading of files, via drupal_function_exists(), and
2. auto-loading of class and interface hierarchies, via the SPL autoloader.

Those are two radically different things. While I happen to think we could easily live without (1), (2) is very important for us to go forward.

One way of accomodating that, is to go back to the hook based approach that Crell drafted in the autoload module. Something like that could work:

/**
 * Implementation of hook_autoload_info().
 */
function example_autoload_info() {
  return array(
    'functions' => array(
      'example_admin_*' => 'example.admin.inc',
      'example_page_*' => 'example.page.inc',
    ),
    'classes' => array(
      'ExampleClass' => 'example.classes.inc',
      'ExampleInterface' => 'example.classes.inc',
    ),
  );
}

Note that it cannot be based on the information in the .info, or we go back to the issue described in length by Jacob in #50. And no, we don't want to parse all .info files at each request. No, we don't.

Making the registry more fault-tolerant I am all for. There's already an issue to help address Jacob's issue: #532010: Use require instead of require_once in _registry_check_code(). I think an "if that fails for some reason, rescan the registry and try again once" is a reasonable attempt to make. chx has also proposed a rescan.php file (or similar) that would rebuild all of Drupal's semi-caches; menu, theme, registry, etc. There's some sort of lock down logic needed there, of course, for security but I think that is solvable. A flag in settings.php would work as well.

The naive name==directory class pseudo-namespace structure taken by PEAR or ZF is actually entirely incompatible with Drupal's approach to modules. I've had that debate with some of the more "traditional" framework folks, too, who want to make that a de jure standard for PHP 5.3 with namespaces.

While we could replace the registry with a manual hook a la the autoload module, that is an enormous burden on module developers. With the registry, as a worst case you kick Drupal really hard in the right place and it re-finds all of your code. With a manual approach, you have to maintain a by-hand index of every piece of code in your module, and we have yet another gigantic static cache array we have to reserve memory for. Even if we only did that for classes, think of a module like Views. That would be a horrifically large hook to have to maintain.

Plus, such a naive approach would result in dozens of chained autoloads on deep class nesting. Imagine class A extends class B extends class C extends class D. If you create a new instance of A, that will trigger an autoload on A, which will in turn trigger an autoload on B, which in turn triggers an autoload on C, which in turn triggers an autoload on D. With proper caching and front-loading in the registry (which I still feel we need to re-add for this reason), we can eliminate all four autoload triggers or at least cut it down to one. I am not confident that an entirely manual approach will be able to do that.

Sorry, correction, this is the issue I meant: #540402: Use include_once instead of require_once in registry

chx, I am reopening that issue. It's a perfectly straightforward and simple fix that doesn't break a recovery script.

#534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap has a recovery script that works for rebuilding the registry in situations like described by Jacob in #50.

I don't understand how you using include instead of require is going to help the situation I described.

I don't think there is a way to "autofix" this. Also, using include instead of require is faulty. Say I upgrade my site and it is a busy site. People are submitting forms all the time. If I cause a registry problem, which causes registry to somehow know it is broken and then rebuild itself, it will cause the form submission which triggered this to re-submit itself I suppose. This could cause data integrity issues. Further, regsitry rebuilding take a long time. So does that mean you will lock all users out of the site while rebuilding happens? Or will we get into a race condition where 100 processes are all trying to rebuild it at once.

I agree w/ Crell that DamZ's proposal is too much of a burden on developers.

Here's another idea:

Keep Drupal using easy to understand PHP with a path of include statements you can follow.

Implement the page split and other performance improvements

Define what our benchmark is for $10 hosting explicitly and test it. Maybe you can run a site with 1 admin and up to 10 concurrent authenticated users who get logged off after 10 min. Anything more than that, and you need more h/w. Is that so bad?

The answer really is "get an opcode cache" IMO, and you can get one on shared hosting now for about $20-$25/mo. Hell, I bet it's a good business to get into offering it for $15 and someone will do it.

In conclusion:

Scaling is important, being fast under all conditions for all price points is impossible. This does not increase our scalability (because has little / no benefits over a proper APC cache), hurts deployment (because new files / removed / moved files can not be taken care of safely) and destroys the developer experience (for the obvious frustrations myself, everyone on my team, and others in the community have voiced).

Can we focus on making it optional at least? So that it can be turned off for sites in development, sites with APC and sites which have to do hot deployments in mission critical environments can disable it?

Jacob, user count is not the issue. A site with one single user per hour that has Views, CCK, a couple of multimedia modules, Panels, and Ubertcart installed (which is not at all an unusual setup, even for a low-traffic site) is going to crash and burn from hitting the PHP memory limit. It's going to spend more time just loading and parsing code it never uses than it will on database traffic, making a single page load for a single user longer than it has any right to. That is the problem. Yes, I have run into that already in Drupal 6.

You may think that's not an issue, but it is. If the low-end and mid-range markets can't use Drupal, they'll find an alternative (WordPress, Joomla, Plone, SharePoint... We do have competition). And then rather than switching to Drupal when they have the budget for a dedicated server and opcode cache, they'll stay with those alternative systems and put resources into that project instead. We'll have lost users and developers forever. I do not want Drupal to be a high-end-niche platform. Those are the platforms that die.

An an opcode cache is not a silver bullet, either. Even on a well-tricked-out dedicated box, if you burn 50 MB per page request on a 2 GB server, you can serve a maximum of 40 requests at once. (Actually less since the OS, DB server, Apache itself, etc. will eat up memory as well.)

However, per-hook-manual-registry (like we have in Drupal 6 for theme and menu systems) is no better than one giant autoload hook. You still have to manually specify every function or class in your module. You're just specifying it in 50 places instead of one. This is still not an improvement.

Even if we went with the otherwise stupid plan of "make everything OO so that we can use PHP autoload", we'd still need the registry to find classes in the first place.

Switching to include_once() is to make the system more fault-tolerant so that it doesn't WSOD. That's separate from a self-correcting system, which as you correctly note is non-trivial to write but is still potentially valuable to have if we can get it right.

I am now more for removal. Class autoloading is not that hard, just have a hook in .module that tells about your classes. There are not many. The whole of includes has 48 classes including DBTNG, file etc.

Core has 48. Views alone adds a few dozen. My understanding is Panels wants to move in the direction of Views and use more classes. There are plenty of other contrib modules that use classes. Do you really think Views needs another 500-line hook? It has plenty already. :-)

Looks like a great start - my only thought was that ideally we'll be able to do away with tokenizing the files in favor of something faster.

Is it just me, or does this look ridiculous....

function drupal_function_exists($function) {
  return function_exists($function);
}

I assume there will be follow up patches?

So what exactly does it buy us to remove the registry (for functions)? I don't think the "drupal_function_exists() is more expensive than function_exists()" can really outweigh the potential performace increase for non-opcode sites. Thing is, all of this stuff is really hard to benchmark, especially without contrib. The registry will have a *much* bigger effect with an actual site that uses lots of contrib modules. Think of a common site that has ubercart, views and panels - here we can really save a lot of code parsing (e.g. just think of all the ubercart code that is only needed in the cart section of a site).

I'm all for adding a recovery script to core, that's why I opened #534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap. With something like this added, I don't see why we should remove the registry - we're discussing benchmarks vs. benchmarks, but unfortunately withoug actual, proper benchmarks. Such benchmarks are admittetly hard to do, especially as we don't have many contrib modules for D7 yet. But still, if the reason for removal is the claim "On a D7 site, the penalty by drupal_function_exists() vs. function_exists() outweighs the potential memory savings by parsing a lot less code", then I am very unsure if it's wise to proceed with removing the registry.

For reference, after talking in IRC we want to keep class/interface autoloading. You can read a simple example of this at http://www.brandonsavage.net/making-life-better-with-the-spl-autoloader/ or see it in a drupal module with a hook at http://drupal.org/project/autoload

This is a feature in php5 that we should use.

So, the efforts to remove the registry should focus on functions at this point. If anything, change how the classes are defined and/or how the information is stored.

But still, if the reason for removal is the claim "On a D7 site, the penalty by drupal_function_exists() vs. function_exists() outweighs the potential memory savings by parsing a lot less code", then I am very unsure if it's wise to proceed with removing the registry.

Given that the patch removes the registry but leaves the page split intact, it certainly seems like it shouldn't increase the amount of code parsing/memory usage at all (or at least if it did, it could be easily rewritten not to do so). Or am I not understanding something fundamental here?

Agreed with @pwolanin that it would be nice to remove the tokenizing -- though it's a small point in the grand scheme of things, it makes the Drupal 7 installation painfully slow to endure. I guess that would mean exploring the other options mentioned above for class/interface autoloading.

@catch: I think your latest version of the patch has some other unrelated changes mixed in by mistake?

@David_Rothstein: Well, up to now, we haven't optimized Drupal core to actually make use of the registry in terms of parse savings. If we did, we would e.g. move away all the registry style hooks. And contrib would do the same. That's where the parse savings would come from. Crell did some very compelling benchmarks a while a go on Drupal 6 (see http://www.garfieldtech.com/blog/benchmark-page-split).

@Frando - there is an easy solution - move all the hooks to something like modulename.hooks.inc.

module_invoke_all() can load this file if it can't find your hook implementation - this is a trivial way to get uneeded code (e.g. hook_menu, hook_actions, etc) out of .module.

Even simpler than modulename.hooks.inc, anyone is always free to do something like the following, right?

/**
 * Implement hook_menu().
 */
function mymodule_menu() {
  require_once 'somefile.inc';
  return _mymodule_menu();
}

(where _mymodule_menu() is defined in somefile.inc and contains a massive block of code)

Ironically, that is the pattern currently used by the registry system itself....

function registry_rebuild() {
  require_once DRUPAL_ROOT . '/includes/registry.inc';
  _registry_rebuild();
}

Although certainly the idea of auto-loading a modulename.hooks.inc file is pretty simple too, and worth exploring.

The only way to speed that up is to start using standard naming conventions for classes and their locations.

See http://drupal.org/node/395472#comment-1719782 and the following comments for a rather brutal investigation of the matter.

Perhaps a lib directory in each module, which used upercase filenames (to match the classname) would be an efficient way to handle it.

-J

1. The registry allows to split of AND dynamically (automagically) load integration code for other modules that may not be installed. Currently, contrib modules either need to implement their own API layer to allow third-party modules to integrate with their API without negatively affecting the site performance of many other sites that do not have that particular module installed and thus don't need to load the code at all (such as hook_panels|ctools_include_directory() or I think I've heard from from $module.rules.inc). The registry empowers Drupal developers to advance on Drupal's modularity (and do it properly).

   This is a major issue for contributed modules and most probably one of the primary reasons for why we have weak cross-module integrations and so many totally needless "glue" modules that only implement a module's API on behalf of another module.

   If we consider to drop the registry, then we have to ensure this highly valuable feature through a different mechanism.

   A $module.hooks.inc is no solution, because that file would be loaded on 100% of all requests.

2. We certainly can achieve a vast decrease in memory consumption with the registry by ensuring that files are never loaded but only read to scan for functions when the registry is rebuilt. I think that reducing the overall memory consumption on production sites is the registry's primary purpose (where code changes occasionally only).

   Considering the time spent in PHP, my benchmarks for the total module split resulted in a 20-30% decrease of both execution time and memory consumption (on APC) -- with all Drupal core modules enabled only. Common Drupal sites have a lot more modules + code to load.

3. We still have a bunch of patches in the queue that advance on the registry (such as making theme() fully leveraging it, too). Removing it now is like removing fields in core now, just because the implementation is incomplete.
4. If we remove the function registry, then I see little point in keeping the D6 page split either. The purpose of both is identical.

Even if we only use the registry for classes, it's worth still keeping the function index in place. It's already written, and a contrib may be able to leverage it. We just need to remove the drupal_ from function_exists().

Class-per-file is a very stupid way to organize PHP code. We also need to account for deeply nested class hierarchies with front-caching, which can avoid a huge number of autoload lookups.

we could put classes to file relationships in the module .info file to avoid having to parse them out of the source. etc - there are many options. I think that's far easier to get people to accept than an arbitrary file naming convention - especially if that means we need to deal with stupid CVS file move/rename limitations.

@sun you APi can be public if callers use module_invoke_all() or module_invoke() - as suggested by catch, that would let us stuff rarely-called hooks outside the .module file. The point is that hooks called frequently would stay in .module.

Another thing you can't do anymore w/ the registry, I just found out...

Remove a module you have enabled from the filesystem: KABOOM. Recommended practice? No. But it's going to happen while you're upgrading a module if you follow the steps outlined in UPGRADE.txt. Better hope to $higher_power that someone doesn't ping your site in between your deleting the old files and putting the new ones in place. :P

@webchick: Amazing you haven't noticed that you can't remove mods till now!
#498412: No way to access site if modules are deleted or disabled without using form
This is especially relevant when people want to move mods from sites/all to sites/$site or whatever

Also, unrelated but equally nasty: If you make a typo in hook_menu (try MENU_NORMAL), your site is completely screwed unless you edit database tables. I'll make an issue for this if no one has one on hand.

@catch:
1 class per file: Okay to disagree, but also I think it's pretty naive to call it "incredibly stupid" when it is the convention used by almost every programming language on the planet (and by many or more popular PHP frameworks). Doesn't mean we have to do it, but I think randomly naming a file based on the "general feeling" of the classes inside as we do now is equally or more stupid.

@pwolanin: Defining classes and their relationship in the info file sounds like trouble to me, but open to seeing what benefit it gives us.

@everyone:
DO we need a class or function registry? The class registry is not going to buy us very much the way Drupal is now. There is NO WAY to make an efficient autoloader without a filenaming syntax unless you are able to load the definitions from a config file somewhere (which still has overhead). Seriously, how would anything we build be any different from the current registry?

You can't recover, right? Or you have to load and parse files to make sure you aren't out of date. I'm much more in favor of making a hooks.inc file which can include its own libs inside function scope as needed upon invocation and a pages.inc when your menu handler gets called if we want to cut down on kb loaded.

Best,
Jacob

An alternative:

• fully remove registry
• keep file definitions in .info files
• By default include all files at bootstrap; developers don't need to bother about function/class dependencies and have all freedom in code/api organization, but is bad performance)
• Create a pluggable "compiler" that generates a optimized version of your drupal installation (store in cache dir)
  • sites on servers with opcode caching could use a compiler that combines all files into one file.
  • sites on limited webservers could use a compiler that split's code into several files (have a look at that dcc.module I posted in #363891: Optimize performance using code generation).

@JacobSingh - At the very least we need a class/interface autoloading system. Here's an example to illustrate. Currently Views is OOP with quite a few classes and most of those are extensions of other classes. There are a number of modules that extend views that all use classes. In Views 2 there is a system that tracks all the classes and their dependencies to load them and work in PHP4. This system already requires us to define stuff and does a lot of work PHP can do for us.

If we use the PHP5 spl autoloading system it should be less code for developers and faster (it's C code rather than PHP).

Sure, this could end up in views or ctools so those modules could use it. But, I expect more OOP with drupal now that PHP5 is required. I have already heard people talk about it. It would be good to provide a generic system that all modules can use.

At the very least this could be a hook (something like hook_autoload or hook_classes). If views defines 20 classes it would only need to be a 26 line hook (including the header comment block). Even if we remove the registry something like this could be very useful.

@casey - I like the idea of having a compiler that generates one file for opcode caches. Removing the requires will save a little on performance. But, that's not going to happen for D7. The code freeze is almost upon us.

We can't include all files at bootstrap on shared hosts. That would use a lot of memory and would be a problem. More users run drupal on shared hosts than anywhere else. We need to keep shared hosts in mind.

@JacobSingh
re:

There is NO WAY to make an efficient autoloader without a filenaming syntax unless you are able to load the definitions from a config file somewhere (which still has overhead).

I thought this is more or less what I was suggesting with putting class to file relationships in the .info file. We already incur the overhead of parsing those files, so hopefully this would not be more more.

Requiring a declaration of all classes in the info file is worse than requiring a declaration of all files, IMO. It's more manual work unless you're doing the naive class-per-file approach, which we need to get away from. While it's true that right now most modules do not have any classes, I do not expect that to remain the case indefinitely and plenty of modules will have many classes. (Think Views, views plugins, and if merlin converts Panels to be more Views-ish has he's mentioned in the past any Panels-related modules.)

That also doesn't address classes in Drupal core, of which there are a couple now and will be more as time goes on.

@Crell - listing a class in the .info file is a one-time cost. So, I'd personally rather add a slight cost to DX and have a stupid-simple class auto-loader with no file parser versus the alternative of actually parsing files.

Parsing is not a regular cost. It only happens on total rebuild, which is an extremely rare operation. The parse cost itself has no impact no normal page activity.

Discussing with Jacob - we handled variable class names in a factory function in the Solr suite like:

    list($module, $filepath, $class) = variable_get('apachesolr_service_class', array('apachesolr', 'Drupal_Apache_Solr_Service.php', 'Drupal_Apache_Solr_Service'));
    include_once(drupal_get_path('module', $module) .'/'. $filepath);
   $instance = new $class;

Which is a little ugly, but not hard.

@pwolanin listing a class in a hook makes more sense than a .info file. In you rename a class you need to do a total rebuild. This is one of the complaints about the current system listed above.

A separate variable for each class is seriously broken, and means we don't get the ability to use PHP autoload. If we must do it manually, the autoload module already has this question solved. I still disagree with dropping back that far, but if we get rid of automation entirely then that's the way to do it. It's like a 2 hour job to roll and commit that patch.

@Crell - the variable is for classes we can override in a factory function or the like (simple cases). In other cases, you'd just include_once

@pwolanin a much cleaner way is to register all the classes with the spl autoloader and than use a variable to store a class name. Either the default or what is overriding it. The way the spl autoloader works every module can have it's own autoloading function. But, it would be better if we centralized it.

What mfer said. "What class to use here" and "where can I find this class" are by design (of PHP) separate questions. They should not be merged needlessly. A module should just say new $class, and then trust that the underlying system (registry, autoload module, whatever it is) will load the class if necessary. You then don't need to explicitly include_once() anything. PHP handles that for you.

ok, yes - let's just figure out why tests fail.

catch: If this patch removes the registry entirely, then it also removes autoloading and we need another solution. If it keeps the registry for classes and ignores it for functions, then nothing more needs to be done. If the registry doesn't get removed by freeze, then adding another autoload mechanism is redundant and I can't see it getting in until this is removed, but then we're right down to the wire for freeze. So since we seem to agree that we at least need a standard autoload mechanism of some kind, I'm not sure how to proceed here in this issue or another.

There's a lot of back and forth here about the ultimate way to do class registration/autoloading (which is something that basically everyone agrees is needed). Each suggested approach (files-per-classes, a "registry"-style hook, some sort of relationship metadata in .info files, etc.) has its pros and cons, and more importantly its DX cost.

The existing registry's auto-registration feature is quite nice... except when it breaks. :P By removing functions from the equation, we make the window of breakage far smaller than it otherwise would be, and we can re-evaluate at that point (after benchmarks, etc.) whether it makes sense to go further. Though I tend to think that, especially at this late stage in the game, we should be aiming to make the least invasive changes possible unless deemed absolutely necessary.

a first pass at removing drupal_function_exists()

Bot, what do you think?

I'm pretty sure the bot just gave you the finger! :)

Huh, well - I didn't put it to needs review cuase I knew it was still failing massively.

Somehow the right modules/paths are not available in simpletest - trying to debug...

Let me drive this home.

And what about this?

Another round. Stream wrappers were registered after the ini_set('error_log', file_directory_path() . '/error.log'); call for simpletest. Bad, bad move.

Let's see the failures now.

This fixes node display by adding field.default.inc and re-adds the bootstrap column we removed to fix bootstrap calls. I have added watchdog to bootstrap_hooks otherwise what's the point in having watchdog in bootstrap.inc? system_test calls watchdog() in hook_boot and fails otherwise.

Bah!

Fixed image stuff and mimetypes.

I really wanted to get further but I am unable to remain awake any longer. There is not much left: two aggregator tests, simpletest functionaly, Translation export, Token replacement. Role permissions, module API.

subscribe

1) Why rewrite the parser? It works now, it just gets more data than we'll be using. Rewriting the parser means using one that we haven't been debugging for the past year.

2) It occurs to me that we never did get around to un-disaster-area-ing module_list(). It looks like this patch adds yet another obscure boolean parameter to what is already a horrifically evil and obtuse function. Is that really necessary? If we're going to touch that function it should get better, not worse.

Fixed conflict in menu.module in last patch due to LOCAL_ACTION change.

subscribe

Working on the last fails.

chx is having trouble posting to d.o - here's his latest patch.

Note - some code is commented out due to critical bug #535564: D7UX IA: actionable top level content item

oops, wrong file maybe

oops, this patch contain already-committed fixes for debug code in token. That caused a conflict.

Now?

A little bit refined. If this passes, I would like to see a speedy commit.

Sorry that was the wrong patch :/

tobiasb points out that xmlrpc() needs a restore too.

some clean up of doxygen, plus remove ~4 unneeded bare calls to function_exists() that were replacement for drupal_function_exists where it was used to load files contained the desired function. Also, one user test had a return; added at the beginning, which is probably not desired.

As long as this passes all tests, I think it's good to go.

Seems like the # of tests is a little lower than I see for other patches since some of the registry tests were removed.

Ok. After quite a bit of drama related to missing commit messages (ugh), I got this committed. Great team work on this, folks.

There is probably documentation in the upgrade guide that needs to be amended as a result of this.

edited this http://drupal.org/update/modules/6/7

follow-up issue here: #557542: Cache module_implements()

Discussion on how this affects AJAX behaviors in .inc at #559526: All AJAX Behaviors Broken for Forms in .inc files

Note to interested parties, the patch that was finally committed here broke drupal_bootstrap() in a very important way. See #606146: DRUPAL_BOOTSTRAP_VARIABLES needlessly loads all bootstrap modules for more.