We (Justin Randell + I ) thought this requires module B to alter the info file of A via hook_system_info_alter but then I now realize that you can't really because you would need to know the path to the other module. Instead, _registry_rebuid needs to call drupal_alter on $files (and send along the result of module_rebuild_cache). Now the module can adjust the 'module' for the files needing adjustment without much fuss.

It so happens this allow boombatower to iterate the list of all modules (m_r_c, see) and sneak the test files into the registry even for switched off modules.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

boombatower’s picture

The conversation started because I need this functionality for #449198: SimpleTest: Clean up test loading and related API .

boombatower’s picture

Assigned: Unassigned » boombatower
boombatower’s picture

Status: Active » Needs review
FileSize
1.12 KB

I have tested this with the patch I will post for SimpleTest and it works.

That should provide a sufficient test for this.

chx’s picture

Status: Needs review » Needs work

the API needs documentation.

boombatower’s picture

Status: Needs work » Needs review
FileSize
2.85 KB

API documentation added.

boombatower’s picture

FileSize
2.85 KB

A few documentation corrections.

RobLoach’s picture

Status: Needs work » Needs review

Subscribing, this is really cool. Might this be moving kind of into the Handlers via files realm (again)?

chx’s picture

Status: Needs review » Needs work

sorry it needs to mention the hook on behalf of other modules too. i suspect that will be a much more common usage.

chx’s picture

Status: Needs review » Needs work

Jimmy, I read on IRC backscroll that it's not clear what i mean. I failed then spectacularly 'cos the original node exactly talks about that. Think of say hook_token , and token module wanting to implement node_token. The registry will look for a function name that begins with "token_" and the rest will be stored as hook. You somehow need to tell the registry that token.node.inc really belongs to node module despite shipped with token.module. So you take the files array and alter

$files[drupal_get_path('module', 'token') . '/token.node.inc'][module] = node;
boombatower’s picture

Status: Needs work » Needs review
FileSize
3.17 KB

Thanks for the clarification.

I've reworked the documentation and included your example.

boombatower’s picture

Removed @see simpletest_registry_files_alter() (will add to other patch)

Added 'dir' key to module_rebuild_cache() array and updated example to use it.

boombatower’s picture

FileSize
3.51 KB

Removed debug and corrected storage of 'dir'.

boombatower’s picture

Needs to store 'dir' key outside of condition so that disabled modules get it as well. :)

boombatower’s picture

FileSize
3.53 KB

Tweaked code style per comments from chx.

chx’s picture

Status: Needs review » Reviewed & tested by the community

That's IMO good.

eaton’s picture

Makes sense to me.

I'll be honest and say that the need for this tweak is indicative of the amount of really confusing complexity that we're adding to Drupal's internal APIs, radically decreasing the transparency for new developers. Drupal modules are starting to look like C++ project directories.

Given the nature of the problem though, I think this directory and info-alter-hook approach is the best one at our disposal. not that many modules implement hooks on behalf of other modules, and they're often doing tricky stuff anyhow.

chx’s picture

See registry allows a number of cool things bu this edge case became harder. There is a lesson to be learned here...

Berdir’s picture

Just an idea, would maybe a standardized naming scheme help for that feature?

For example, if your module "module_x" would like to implement a hook for "module_y", it could place that inside a file

module_x.module_y.inc (or maybe something else, because that is already used quite often)

If the registry now finds such a file, it's automatically added to module_y, without the need of a hook for every single of these files. I assume that modules like token, views and so on do have quite a few of them.

We could still keep the hook for more complicated stuff.

chx’s picture

Status: Reviewed & tested by the community » Needs review

I am quite unsure about that, if someone writes admin.module then all hell breaks lose. However, file names are a richer set than PHP identifiers. Say, token.-node-.inc ... opinions please.

Damien Tournoud’s picture

I hate the naming convention idea. We don't have any type of "magical names" in core, let's keep it this way. Magical naming strongly decrease the transparency of Drupal for new developers (to reuse Eaton's wording).

I strongly prefer the registry altering approach.

eaton’s picture

Status: Needs review » Reviewed & tested by the community

My concerns about the registry system's conceptual complexity aside, I think we're better off sticking with it and using the alter mechanism rather than adding another helping of file name magic. Consistency and predictability is the key, and once the registry is understood, at least, it's consistent and its info data is a logical place to sneak stuff like this in.

merlinofchaos’s picture

Damien: Core is loaded with magic naming. Of course, the rest of your argument is absolutely correct and we should not add more magic naming.

I don't have any particular problems with this, but I want to point out issues I'm having in the Real World with hook_menu_alter() that this system may potentially also run into, with potentially devastating results. This isn't to say that there's anything we can do about this specifically but I want to put the thoughts into people's heads that this kind of collision does happen and we should be at least thinking about ways to mitigate it. I don't have an answer for that part, I just want the scenario understood:

Module A and module B both think they're pretty special and they want to take over a very commonly used page: taxonomy/term/%. Obviously they can't both have it. But it turns out they both modify the menu entry for 'taxonomy/term/%' it in a slightly different way:

Module A modifies 'page callback', 'file path' and 'file'.

Module B then goes and modifies 'page callback, 'module', and 'file'.

You'll notice that because Module A and B were ignorant of each other, the final result is that we have module B's 'file' and module A's 'file path'. This leads to attempting to load the file from a completely incorrect directory, which of course causes a nice crash. I can see similar issues happening with this registry alter if we're not careful.

My best guess is that the only real answer is 'convention', that we need to state very plainly in the documentation that any module using this alter should inspect the data before changing it, to ensure it is what they expect.

Other than that, I don't see any particular problems with this, and in fact Views may end up needing something like this since it does do a number of hook invocations on behalf of other modules, even though they're invocations of its own hooks.

boombatower’s picture

Another note, the magic file names method will not solve the problem that SimpleTest has. That being adding files for disabled modules.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Let's get some tests in here to ensure this functionality is working as expected. Feels very edge-casey to me which means we're likely to accidentally break it in the future unless we have tests telling us we've done so.

webchick’s picture

Also, in terms of a patch review...

+  $dir = $module_cache['token']->dir;

We don't name variables "dir". The file property we're reading is 'filepath', so why not that?

Also, why is dir randomly an object property, rather than another array index? According to the PHPDoc for $module_cache: "The array contains an additional key, 'dir', added by _registry_rebuild()." which makes it even more confusing.

The PHPDoc generally for hook_registry_files_alter() is technically accurate but is missing a "why" piece, IMO. Tell me a story about why I might need to use this hook.

The $module_cache provides the info file information that
+  // contains the list of files that can be used to add files provided
+  // by disabled modules.

That sentence has the word 'that' and 'info[rmation]' like 50 times, and I'm still not quite sure what you mean here. ;) Can we try re-writing this in both places?

boombatower’s picture

Status: Needs work » Needs review
FileSize
4.35 KB

The $module_cache contains an array of objects, so it should be an object property dir. I do however need to correct documentation.

[aggregator] => stdClass Object
(
    [filepath] => modules/aggregator/aggregator.module
    [filename] => aggregator.module
    [name] => aggregator
    [type] => module
    [status] => 0
    [schema_version] => -1
    [weight] => 0
    [old_filepath] => modules/aggregator/aggregator.module
    [info] => Array
    (
        [name] => Aggregator
        [description] => Aggregates syndicated content (RSS, RDF, and Atom feeds).
        [package] => Core
        [version] => 7.0-dev
        [core] => 7.x
        [files] => Array
        (
            [0] => aggregator.module
            [1] => aggregator.admin.inc
            [2] => aggregator.pages.inc
            [3] => aggregator.fetcher.inc
            [4] => aggregator.parser.inc
            [5] => aggregator.processor.inc
            [6] => aggregator.install
        )
    )
    [required_by] => Array
    (
    )
    [requires] => Array
    (
    )
    [sort] => 0
    [dir] => modules/aggregator
)

Chx suggested we set dir since otherwise need to call dirname($module->filepath) a number of times in _registry_rebuild() and hook_registry_files_alter(). The variable is called $dir in _registry_rebuild() so for consistency we use same name. (can rename/remove it)

I attempted to clarify the documentation further.

I am not sure we need a test for this bit of functionality for two reasons. Changing what module the file is associate with uses the same code that all functions work with, so if it breaks Drupal will cease to function. Secondly as I stated above I have a "followup" patch (the reason I am writing this) which will test the other use case. I will add a reference and additional documentation to the API documentation in that patch. I have confirmed that SimpleTest works with this applied. #449198: SimpleTest: Clean up test loading and related API

frankcarey’s picture

Good work folks, this will be very handy for integration modules i have that implement stock integration on behalf of other modules. If those modules do the integration themselves (which is my hope) or another module does the integration hook better, will it be able to take precedence over my modules hook function?

boombatower’s picture

The module will either have to remove your file containing the implementation of the hook(), using hook_registry_files_alter(). Which hook wins, by default, I would assume depends on module weight.

Something like the following should allow the "other module" to override the "API module's" hook.

function othermodule_registry_files_alter($files, $module_cache) {
  $dir = $module_cache['apimodule']->dir;
  unset($files["$dir/apimodule.othermodule.inc"]);
}
frankcarey’s picture

thanks boombatower,

I guess I could do the leg work on my end then, and only load the integration include if it hasn't already been implemented by the proper module?


If (!in_array('integration.inc', $module_cache[proper_module][info][files])) {
  //not used yet, so go ahead and load
}  
  

I guess this is more for "aggressive" modules that need to unload another modules file, and less for a more "passive" module just trying not to step on another modules toes? For instance, another module that wants to do things *better* than mine is, would use this unset method? :)

boombatower’s picture

Yea, and your example should work fine. Add the integration file to the .info, then unset it is other module has file...otherwise you would need to manually load it (I think the integration file should be in the .info file).

I would probably leave the unsetting of your implementation to the actual module, as it then does not require you to add conditions for every module since you are defining the API.

If however you are just override a core hook or something then you may want to do it conditional.

Anonymous’s picture

Issue tags: +Registry

tagging with Registry.

Anonymous’s picture

in #drupal IRC chx asked me to roll the tests for this. thing is, i'm not sure about the approach we're taking, so i'll try to outline my concerns and see what people think. i think the 'change the file' approach is potentially harmful, so i wonder if we shouldn't setup a hook to allow modules to alter the resource rows in the registry directly.

there are two overall use-cases here, both very different. the first is 'alter what file the registry loads for a hook implementation' and the second is 'tell me about all implementations of something regardless of whether the module is enabled or not'.

for the 'implement a hook/override a hook' use case, if we're trying to get the registry to load a different file, why are we so focused on getting the registry to parse a different file? why not just alter what the registry will load?

if you replace a file from the list the registry will parse, then we end up with side effects. or, we end up with restrictions like 'don't put more than one resource in a file'.

take the example supplied in #9 for token module. what if token module were to provide a file called 'token.hooks.inc', which contained implementations of hook_token for 'foo' and 'foometoo' modules. when we do:

$files[drupal_get_path('module', 'token') . '/token.hooks.inc']['module'] = 'foo';

then in _registry_parse_file here:

        // Collect the part of the function name after the module name,
        // so that we can query the registry for possible hook implementations. 
        if ($type == 'function' && !empty($module)) {
          $n = strlen($module);
          if (substr($resource_name, 0, $n) == $module) {
            $suffix = substr($resource_name, $n + 1);
          } 
        }

we collect the wrong suffix for 'foometoo_token', and module_implements wont return 'foometoo' as a module that implements hook_token.

but wait, there's more. look at the examples for #27 and #28. any time a module tells the registry to parse file a.inc instead of b.inc so it can implement/override a hook, if there are resources in b.inc that aren't in a.inc, then we have missing resources.

the simpeletest use case looks different. there the requirement can be met by having the registry parse the files for all modules, enabled or not, and just flag the resources appropriately. (that's actually what i implemented for #340052: speedup simpletest by preloading the registry, but it didn't make it in.)

if the registry parsed all files for modules enabled or not, and we add a hook to allow modules to alter the resource rows, we wont force any of the per-file constraints.

chx’s picture

I disagree. I think that implementing hooks on the behalf of another module is such a rare edge case that we can tell people to use one inc file per module (views certainly does that). Calling a hook for each of the thousands of rows in the registry is eeek! especially that we need to make sure that while the registry is rebuilding, module_implements uses its maintenance variant.

Anonymous’s picture

in #drupal IRC, chx asked for some code, so here goes: http://drupalbin.com/9373

edit, pastebin was wrong, please ignore, i'll paste the code here:

  // in  function _registry_rebuild()
  $parsed_files = _registry_parse_files($files);

  // Allow modules to alter which implementation of a hook is loaded by the registry.
  module_invoke_all('registry_alter');

token implementation (pun intended):

function token_registry_alter() {
  // We want to change the module and suffix entry for 'node_token'.
  db_update('registry')
      ->fields(array(
        'module' => 'node',
        'suffix' => 'token',
      ))
      ->condition('module', 'token')
      ->condition('type', 'function')
      ->condition('name', 'node_token')
      ->execute();
}
chx’s picture

Ouch! Encapsulation gone. Why would any module operate directly on a db table? Thats not nice. I still think my original implementation is good enough.

chx’s picture

Re-reading the issue it turns out Justin was trying to cover #27 -- #27 is another issue. I do not want to deal with that now. Eventually when pluggable subsystems are fixed you wont be able to do that anyways -- which the same as in D6, you cant have two functions with the same name.

Anonymous’s picture

#34 was trying to address 'Calling a hook for each of the thousands of rows in the registry is eeek!', to show that we don't need to call it per row. my pseudocode example certainly leaks the registry's innards, but its not hard to see that we could implement it without that.

my issue remains - if we're trying to allow modules the ability to alter a registry record, why not have a hook that allows modules to do that, rather than monkeying around with the $files array? even if we're not going to allow the use case at #27, i still think we should make our implementation more obviously match what we're doing, and not introduce the 'it has to be in one file' limitation unless we are forced to.

boombatowers' issue is a different one as well. for that, i think we should make the registry parse files for uninstalled modules, and mark the resources from those modules as not enabled, but that should be another patch, based around this patch from #340052: speedup simpletest by preloading the registry.

Anonymous’s picture

just thought of another wrinkle. currently, we load and parse files for all enabled modules. if a module implements a hook on behalf of another, we need to make sure that that both modules are enabled, not just the implementing module, else we'll end up calling a hook for a disabled module, which would be a Bad Thing.

so, if we go with the file-monkeying approach, the implementing module probably needs to do something like:

function hook_registry_files_alter($files, $module_cache) {
  $dir = $module_cache['token']->dir;
  if (module_exists('node')) {
    $files["$dir/token.node.inc"]['module'] = 'node';
  }
  else {
    unset($files["$dir/token.node.inc"]);
  }
}
chx’s picture

but its not hard to see that we could implement it without that. <= For sure... but I wonder how do you really intend to do that. In general, there are three places where you can call a hook: per rebuild, per file, per entry. If you do not want to enforce that one file belong to one module I cant imagine how will you get away with that without firing a hook per entry unless you require people to give you a data structure like node_token => node.

A better approach would be to introduce the hook registry finally and use the DFS code to do per hook weighting.

Anonymous’s picture

"unless you require people to give you a data structure like node_token => node"

yes, that's pretty much what i was thinking. that feels pretty straight forward, and would allow for the registry to just loop over the returned array and update registry entries.

"A better approach would be to introduce the hook registry finally and use the DFS code to do per hook weighting."

can you point me at some links/discussion about a hook registry? i'm sure i've heard it before, but i can't remember the details.

chx’s picture

I am not sure there is an issue for the hook registry. There is already hook_hook_info we might want to simple reuse that however that creates a tab in the trigger ui for every module so then that needs fixing if you have too many modules defining hooks.

Anonymous’s picture

updating this after a chat with chx in #drupal.

the consensus seems to be that modules that want to implement a hook for another module should signal this in hook_hook_info.

the registry will use this information to correct the file-based module and hook guesses it made during the build.

either chx or i will pick this up, depending on who gets to it first.

chx’s picture

Assigned: boombatower » chx
FileSize
5.9 KB

Well. There is a lot simpler solution. We can simply try to match the module name with the beginning of the function name. Running a preg is only a little bit slower than running two string operations (less than 10 percent) and as there are a lot of DB operations, the slowdown is not measurable. No new tests are needed. No explicit registration is needed.

This has interesting consequences: files in ./include now can implement hooks so if we want (I think we want) we can move say system_elements to form.inc and so on.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

So, system_test and system module -- we now do an rsort so system_test comes first in the preg.

sun’s picture

On rsort():

"... to prevent function name clashes between modules sharing the same prefix."

RTBC after that ammendment.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6.05 KB

Added comment no other change so RTBC per sun's comment.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs work

the rsort just makes the way the registry will guess at hooks and modules predictable, but not in any way guaranteed to be correct.

given two modules called 'views' and 'views_attach', and somes functions that match 'views_attach_*', we can be sure that the registry will guess that these functions belong to 'views_attach'. nice and predictable, but not in any way a guarantee that 'views' doesn't implement any hooks that start with 'attach'.

we're still guessing, but we've just shifted the way we guess. actually, this makes the probability of these guesses being wrong greater than the current scheme, because we'd be checking every single 'views' function against 'views_attach'.

not sold on this idea. a glance at the list of module names shows lots of modules that have a base module and many related modules with the base module name + '_' + stuff:
cck
og
drush
views
event
filefield
flag

not an exhaustive list, but you get the idea.

Anonymous’s picture

Status: Needs work » Needs review

maybe that was too harsh.

the regex scheme is better than our current guesses in that it allows implementations from other modules. we have the same issue in D6, and unless we registered hooks, we're always going to have this issue. so, setting back to needs review for further discussion.

chx’s picture

Assigned: chx » Unassigned
Status: Needs review » Reviewed & tested by the community

Actually, if you create such a mess then you made your bed and you sleep in it. Let's say we have Drupal 6, views and views_attach modules and you have a function called views_attach_boo. Now, what do you think module_invoke_all(boo) will do? Happily fire views_attach_boo because D6 has no clue at all about which module defined which function. We are not worse off than D6 was and short of explicit hook definition we cant do better. And you need very explicit hook definition if you want to make sure views_attach_boo can be [views] and [attach_boo] when its something that views does but also can be [views_attach] and [boo] when views implements something on behalf of views_attach. This is a terrible and sorry mess which we do not want to enter. Avoid such function/project naming, case closed.

Pasqualle’s picture

is there an api example how to implement a hook on the behalf of another module? I think it would be useful..

chx’s picture

Pasqualle, there is no need. Just write function node_token and it will be picked up automatically as in D6 -- the node will be recognized as a valid module.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

I'm with Justin on this. The great thing of the registry is that we resolve the namespace issue. A hook implementation is a function named _hookname(), *in your module*. I don't want to go back to the old way.

Additionally, the need for a module to implement a hook on behalf of another module should be rare anyway. We want to foster cooperation between module maintainers so that this doesn't become a long-term situation (I'm perfectly aware that we have high-profile modules that are in this situation currently, and I hate that).

Let's go back to the registry alteration strategy.

sun’s picture

Status: Needs work » Needs review

A hook implementation is a function named _hookname(), *in your module*. ... Additionally, the need for a module to implement a hook on behalf of another module should be rare anyway.

Let's see whether this is feasible and/or a sane idea. Picking sites/all/modules of an arbitrary Drupal site:

diff:
node_diff
taxonomy_diff
upload_diff

i18n/i18nviews:
i18ntaxonomy_views_handler_field_allterms

pathauto:
node_pathauto
taxonomy_pathauto
forum_pathauto
user_pathauto
blog_pathauto
tracker_pathauto

rules:
comment_rules_event_info
node_rules_event_info
workflow_ng_action_node_publish_upgrade
path_rules_condition_info
php_rules_evaluator
system_rules_event_info
user_rules_event_info

token:
comment_token_values
node_token_values
book_token_values
taxonomy_token_values
user_token_values

views:
book_views_data
comment_views_data
node_views_data
poll_views_data
profile_views_data
search_views_data
statistics_views_data
system_views_data
taxonomy_views_data
translation_views_data_alter
upload_views_data
user_views_data

Note that I only copied one hook implementation on behalf another module here, so most function names above stand for one file containing multiple hooks on behalf another module.

Damien Tournoud’s picture

@sun: in #53, you will see "I'm perfectly aware that we have high-profile modules that are in this situation currently, and I hate that".

Please don't get me wrong: I'm not arguing we don't need this patch, nor that it is reasonable to forbid modules to implement hooks on behalf of others. I'm just saying that the current flat-namespace causes all sort of crazy troubles. We don't need to make that easy. The registry-altering solution outlined in previous posts sounds perfectly reasonable for me.

Berdir’s picture

I agree that we would lose one of the advantages of the registry system with an automatic solution. That is the almost the same type of magic as my suggested naming convention above.

From the discussed solutions, a optional "hook info" registry hook *only* for foreign hooks (hooks that don't belong the the module) sounds imho pretty nice. hook_hook_info() would be the most obvious name though that is already used by trigger (that should imho be called trigger_info or something like that anyway).

sun’s picture

- Usually, we allow developers to use _alter() to manipulate existing information that was collected. The proposed hook is intended to add, not alter.

- As of now, the registry does not need to load modules and needs no PHP code in modules.

- Can we move this additional registry info into .info files? (see also #399702: Registry: replace files[] with include[]/exclude[])

Crell’s picture

chx asked me to weigh in here. I've not read the entire thread yet, but I've skimmed it.

I have to agree with Berdir up in #18. The page split work for D6 already establishes a pattern of owningmodule.descriptor.inc. When I was working out how to name the files, I did that deliberately because then we could very very easily derive the owning module from the file name if we later decided to. We should decide to do so. :-)

The added advantage is that the files are then self-identifying as to the module they "belong" to even if put into an arbitrary list... say a list of open files in an IDE or text editor. It also means that you're more likely to have globally unique file names, which is not currently something we leverage but could prove useful down the road.

Another advantage is that it maps very easily to the way namespaces work in PHP 5.3. Whether we eventually use namespaces or not (that's still years away before we can look into it), leveraging similar mental patterns lowers the barrier to entry for PHP developers coming to Drupal.

The logic flow is very simple:

If the file is named: FOO.module -> All code in there is "owned" by the FOO module (in the FOO namespace, essentially).

Else if the file is named: FOO.BAR.inc -> All code in there is "owned by the "FOO" module. (BAR is ignored.)

Else the file is owned by the module named for the enclosing directory (eg, foo/node_types.inc -> "foo").

It's easy to teach, self-documenting, and won't be necessary unless you're doing something weird on behalf of another module, which is very much a minority of modules. It requires no code to be loaded in a module in order to hook up a file properly. The 3rd conditional above also means that most existing modules won't have to do anything differently if they don't want to, unless they are implementing hooks on another module's behalf and NOT following the pattern above, which I suspect is a fairly small number.

Berdir’s picture

Trying to reply to DamZ in advance..

I hate the naming convention idea. We don't have any type of "magical names" in core, let's keep it this way. Magical naming strongly decrease the transparency of Drupal for new developers (to reuse Eaton's wording).

What are hooks else than naming conventions magic?

sun’s picture

Please enlighten me.

given two modules called 'views' and 'views_attach', and somes functions that match 'views_attach_*', we can be sure that the registry will guess that these functions belong to 'views_attach'. nice and predictable, but not in any way a guarantee that 'views' doesn't implement any hooks that start with 'attach'.

we're still guessing, but we've just shifted the way we guess. actually, this makes the probability of these guesses being wrong greater than the current scheme, because we'd be checking every single 'views' function against 'views_attach'.

Given 2 modules

- views_attach
- views

and the preg pattern, which is

[module]_

How can the implementation of hook_attach()

views_attach()

be mistakenly associated with views_attach_ ?

Berdir’s picture

It can't, but..

Given 2 modules

- views_actions (doesn't exist, just an example)
- views

Given 2 hooks

- hook_delete
- hook_actions_delete

Given the function
- views_actions_delete

Now.. to which module belongs that function? and which hook is it?

Edit: first example was wrong, updated..

chx’s picture

Status: Needs review » Reviewed & tested by the community

Here is a list of possible solutions.

  1. Alter hook. Pros: allows for a lot of magic. Cons: per file,you need to register files.
  2. Magic file names. Pros: no code needed. Cons: per file
  3. Optional hook registry in code. Pros: per hook weighting. Cons: code, you need to register hooks.
  4. Optional hook registry in info. Pros: no code needed. Cons: you need to register hooks.
  5. Module name discovery. Pros: it just happens, cons: confusing namespace issues.
chx’s picture

Status: Reviewed & tested by the community » Needs review

Sorry for the status change.

chx’s picture

So, a bigger summary. The current problem is that the registry simply assumes that files listed in the info file of a module implement hooks on the behalf of this module. This is a sane assumption almost all the time -- if node.module lists node.build.inc you expect node_menu there to implement hook_menu and you'd be surprised to see user_menu in there.

However, some contributed modules define their own hooks and then implement them on behalf of core. So, say node_token will be found in a file listed in token.info to implement hook_token on the behalf of node module. This is not recognized by the registry because it expects the function to start with token_ not hook_.

There are various solutions suggested to this problem.

  1. An alter hook. This would let token module alter the files array registry rebuild uses the match filenames with the module they belong to. So $files[drupal_get_path('module', 'token') . '/token.node.inc']['module'] = 'node';. The advantage of this is powerful -- for example it allows the simpletest module to add the test files in disabled modules to the registry. However, there is a hook to be written while in D6 node_token is recognized simply because Drupal 6 does not know which module defined that function. Also, calling hooks while building the registry is a bit hairy -- we would need to make sure module_implements uses the maintenance version not the normal, registry-using version and the alter hook implementation can only be in .module because there is no registry to find it in an include. Not a big deal, though (and now I realize we need to do that probably for the info alter hook).
  2. In my example, I used token.node.inc. If we would call this node.token.inc then it can be recognized as belonging to node module just based on the file name. This not a bad solution, however, as the previous version, this requires one file per one hook implementation -- ie taxonomy.token.inc and so on. Some would prefer to see these all in token.hooks.inc for example. Another minor drawback is that currently every file looks like token.foo.in in there. We do not want token.node.inc however, because if someone adds an "admin" module then suddenly all the foo.admin.inc would belong there? So thats not good, we want modulename.functionality.inc. However, this requires particularly little effort from the developers, it's quite magical.
  3. We can write a hook that registers the hooks -- not all hook needs to be registered, just when you want the 'implement on behalf' situation to happen. We already have hook_hook_info, it'd be rather easy to piggyback on it (and for the hooks that do not make sense as triggers, we simply do not supply a description and actions/trigger wont pick those up then). Now, this is a hook, so this suffers from the problems of the first solution (you need to code, invoking a hook not ideal at this point) but the advantage is that we could allow per hook weighting. This is the first solution which can be used to put multiple hook implementation in one file -- node_token and taxonomy_token are simply recognized based on the suffix _token.
  4. But do we need this hook registry in code? Its not like you need logic to define them, so we can actually define the hooks in info files. The only drawback here is that you actually need to do it. There exists an amorph fear that this might not be flexible enough. There is an info altering hook, however.
  5. Finally we can just look at the taxonomy_token function name and say "oh, taxonomy is a module name, surely this belongs to taxonomy". This is quite like Drupal 6 but then if you have a function called views_attach_view, what's that? hook_attach_view implemented by views or hook_view implemented by views_attach? This is the only solution however where you do not need to do anything, your hooks are simply picked up. Edit: mind you, in Drupal 6 the function would be called for both hook_attach_view and hook_view so creating hooks like this is very unfortunate and wont work at all in D6 so its not sure its a valid concern at all.
sun’s picture

From all those suggestions, 5) would be the nicest, but doesn't seem to work out reliably.

From the remaining, 4) seems to be the most natural to me: We already define files[] in .info files, so it is only logical to place further registry definitions there as well. For example:

files[node][] = includes/foo.node.inc
files[node][] = support/node.php
files[views][] = views.foo.inc
files[views_attach][] = views_attach.foo.inc
Pasqualle’s picture

I will mostly use 1 .inc file per module (but there could be a situation when more files per module or one file for more modules is needed), and I would be comfortable with defining those files in the .info file..

so +1 on #65

there could be still problem if you define something like this:

files[views][] = your_hooks.inc
files[views_attach][] = your_hooks.inc

but that is the developer's problem

chx’s picture

I am in favor of #5 until someone tells me an example that worked in D6 and broken with it.

Anonymous’s picture

"I am in favor of #5 until someone tells me an example that worked in D6 and broken with it."

this is the heart of the matter, IMHO - do we make things 'better' than D6 by getting rid of the ambiguity and explicitly registering hook implementations, or do we keep things as they are?

i'm not sure what i think at this point, but this is the key question. changing it to an explicit system may add a burden on module developers that's not worth the 1% time when this could cause an issue.

if we're going to stick with D6's "if the shoe fits" approach, then i'd be happy with #5, using the list of enabled modules to create a (99% correct) list of hooks.

webchick’s picture

My "gut" preference is #5 as well, although I'm still interested in getting more feedback from people like Damien, etc.

My main reason is developer experience. Registry hooks like hook_schema() and hook_theme() are painful to write. Drupal 7 has made the syntax for database queries more verbose, and now requires manually entering a module's files within its .info file. We are progressively adding more things that get in the way of developers from doing actual development, and cause major WTFs later when you forget to keep your various registries up to speed with the code your fingers are writing.

Being able to register a hook on behalf of another module by simply defining a function that follows the same naming convention as hooks everywhere else in Drupal is a huge boon. Additionally, our "mother" language has very strict rules about name-space collisions. I'm not sure it makes sense to shield our developers from this since it's something every PHP developer is already familiar with anyway.

Pasqualle’s picture

I don't like potential conflicts with module name and hook implementation.
I would like to see this issue #367355: Module names should not contain an underscore fixed for D7. You can say that there is no conflict, but actually it is written in a printed book and many new modules are without underscore just to avoid any potential conflict. It is a shame to not name your module, hook or function as it should be..

New developer does not know what a Drupal hook is, how and why does it work. If you want to implement a hook then first you must define files (with module parameters) in the info file. That description could make the hook functionality much more intuitive..

sun’s picture

Status: Needs review » Reviewed & tested by the community

#64 (chx):

This is quite like Drupal 6 but then if you have a function called views_attach_view, what's that? hook_attach_view implemented by views or hook_view implemented by views_attach? This is the only solution however where you do not need to do anything, your hooks are simply picked up.

Edit: mind you, in Drupal 6 the function would be called for both hook_attach_view and hook_view so creating hooks like this is very unfortunate and wont work at all in D6 so its not sure its a valid concern at all.

#68 (justinrandell):

this is the heart of the matter, IMHO - do we make things 'better' than D6 by getting rid of the ambiguity and explicitly registering hook implementations, or do we keep things as they are?

i'm not sure what i think at this point, but this is the key question. changing it to an explicit system may add a burden on module developers that's not worth the 1% time when this could cause an issue.

Clarification: All of that is talking about the design and limitations of hook namespaces. Even if the registry will load the proper files for chx's example of views_attach_view(), module_invoke() will invoke the wrong functions in the case that both hooks are loaded and invoked in the same request.

Straight to the point: That's a completely different topic and the registry is the wrong place to attack this limitation.

Back to #47.

Berdir’s picture

If the function views_attach_view exists *twice*, and both hook_view and hook_attach_view is executed on the same request, PHP will die with an fatal error. This is a conflict we can't do anything about, not without namespaces (and it will take quite some time before we can use those). Obviously, we should try to avoid such naming conflicts, but these conflicts can arise as soon as hooks use a different amount of words (hook_y and hook_x_y).

However, we can do something about the conflict if the function only exists once. In that case, we can define either automatically (file naming magic) or explicitly (registry hook, alter hook, .info) to which module it belongs (and with that, implicitly which hook it is).

Because there is a difference between D6 and D7, even if we go with #5.
D6: If function views_attach_view exists and the modules views and views_attach are enabled, the hook will be called for *both* hook_view and hook_attach_view. Now, if the amount of required arguments is different, it will already throw a php error. if not, the developer could check the passed in arguments and decide if it is the correct hook. To avoid the mentioned php error, the developer could use func_get_args() and so on.. ugly, but it would work.

D7: If function views_attach_view exists, it gets parsed by the registry and is added *either* to views or views_attach. If the "lucky" module would be views, it would make it _impossible_ for views_attach to use hook_view at all.

Pasqualle’s picture

Status: Reviewed & tested by the community » Needs work

The *magic* match check files in the include directory also..

bug: includes/password.inc

user_hash_password()
user_check_password()
user_needs_new_hash()

are recognized as hooks behalf on the user module..

I wonder why the menu_* functions are not recognized as hooks..

Crell’s picture

Status: Needs work » Needs review

Always assume that your code will be used in a way you do not expect by someone who will not be able to modify your code to make it do what it needs to do... and assume that poor sod will be you.

My gut tells me that if we consciously leave out that 1%, we will hit that problem space immediately after it's too late to fix it. :-) Murphy tends to work that way.

If we can accurate guess 99% of the time, that's fine, but I'd like to know that there is a non-insane way to handle that extra 1% when we need to. At one point I submitted a patch along the lines of #65, except that the module name was optional; if omitted then the "local" module is assumed. chx shot that down on the "wtf" syntax factor. There is also the legitimate concern about too many files; a 5 line hook implemented on behalf of a dozen modules spanning a dozen files will be slower than putting all of them in a single file. If it's a registry-style hook that's no big deal as those are rarely called, but not all hooks are like that.

So I guess that points toward "guess but with an option for an explicit hook instead"... which is actually exactly how hook_forms() works, so it's not a new pattern. The logic would probably be something like:

$hooks = module_invoke_all('hook');
// mutate $hooks to an array of the form:
$hooks['function_name'] = array('module' => 'modulenamehere', 'hook' => 'hooknamehere');

foreach (get_all_functions() as $function) {
  if (!isset($hooks[$function])) {
    // Guess based on module_list() and the function name.
  }
}

// Save $hooks

That would also then be easily extensible with per-hook weights by just adding another key. Cover the 99% with magic and provide a manual method for that other 1%.

boombatower’s picture

Since this popped in my head when I read this I figured I'd comment.

We could implement something more along the lines of #393068: Make the registry class hierarchy, interface and docblock aware (adding docblock support and implementing it seems to alleviate a lot of the registration hook writing the webchick mentioned in #69). This does not solve the SimpleTest problem of adding files for disabled modules, but that could still be done separately (possibly even @registry-always-load, original solution, load all files, etc).

// file: views.node.inc or views.hooks.inc ...

/**
 * ...
 * @module node
 */
function node_views() {
}

NOTE: Still getting back into things after vacation, so maybe I'm just crazy. :)

chx’s picture

This is like a sixth solution which is not a solution as you need to do something and we would need to move half of API module into core to get this working. @Crell, another hook, another solution to a nonexisting problem IMO. Tell me guys really , what do you try to solve? Concrete examples please. We already discussed that namespacing collision are inevitable with PHP (below 5.3 ie true namespacng). We could mandate two underscores between the module and the hook -- maybe that'd be better? That would signal that this is a hook implementation which currently is not clear at all.

@Pasqualle, thats a feature not a bug.

Damien Tournoud’s picture

I like Jimmy's solution. To clarify: by default, a function is supposed to "belong" to the module that defined it (ie. the .info that register this file), but you can override it, by adding a @module directive in the function docblock:

// file: views.node.inc or views.hooks.inc ...

/**
 * Do something silly that I shouldn't do on behalf of the Views module.
 *
 * @module node
 */
function node_views() {
// ...
}
This is like a sixth solution which is not a solution as you need to do something and we would need to move half of API module into core to get this working.

Give me a break here, the patch in #393068: Make the registry class hierarchy, interface and docblock aware is nearly 100% functional, and it is 12kB (most of those being moving code around). Most of the job is done by PHP, the only thing we need is a simple 30 lines docblock parser.

Pasqualle’s picture

wtf? defining the hook's module name inside comment? are you serious? I never heard of any application which used the comments to implement own application functionality..
Drupal should work correctly even if I delete all the comments (docblocks)..

Damien Tournoud’s picture

@Pasquelle: Adding meta-data to the language outside of its own syntax is not new. That method is used a lot elsewhere, most notably in Java / J2EE world under the name of 'annotations'. This is not at all a "Drupalism".

Pasqualle’s picture

Re #76
I am not sure about this feature, because then all functions in the /includes directory are potential hooks, although there are no hooks implemented in those files. And there are some functions already which theoretically could be invoked on special cases. like: image_load(), date_validate(), tablesort_init(), weight_value(), list_themes(), country_get_list(), map_month(), timer_read().

Pasqualle’s picture

As I see annotation and docblock is a different thing.. General docblock in not part of the code, should not be used for application logic..

Crell’s picture

I am familiar with annotations in theory. As of yet I have not been convinced that they are a good idea or a solution I want to use. Mostly I've just seen blog posts declaring that they're a way around PHP's loose variable typing, which I don't see a need for as PHP's loose typing is by design.

Pasqualle’s picture

RE #76
It is wrong to allow hook implementation in any file without explicitly saying that this file implements Drupal hooks.
Third party scripts do not know about Drupal's hook naming conventions, so their functions will be picked up as possible hooks. Or those files should not be declared in the module's .info file?

Damien Tournoud’s picture

@Pasqualle: hooks are only recognized in files you register in the .info file. That's a new feature of Drupal 7. In Drupal 6, all loaded functions that would happen to be named _() where is an enabled module would be recognized as a hook implementation.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6.71 KB

Ok, almost everything after #71 is nonsense. Most of what we have even more complex, baroque solutions to a problem that noone can show an example of (ie where the magic does not work). But, yes, we always let a small door open for the cases we have not though of so I added a hook firing after the registry is rebuilt (no hook invocation problem there) and if you have the mythical module where magic did not work then use that hook and kick the table.

Unless you have an example which can not be solved here, simply do not follow up. I am sure we can dream up even more grotesque solutions but please resist doing so.

chx’s picture

FileSize
7.18 KB

Some documentation about the new hook.

Dries’s picture

From @chx in #64: However, some contributed modules define their own hooks and then implement them on behalf of core. So, say node_token will be found in a file listed in token.info to implement hook_token on the behalf of node module. This is not recognized by the registry because it expects the function to start with token_ not hook_.

The description of the 5 solutions in #64 was tremendously helpful, but ... why can't the registry remember that node_token() was defined in token.module? It sounds to me like the registry's storage model is broken, and that we're trying to work around that in complex ways. How about storing the mapping as it is, i.e. "node_token() was defined in token.module"? I'd like to understand why that isn't possible.

(Once again, this is a hint that the registry might be an idea gone wrong. It's not clean. It might be better to refactor the registry, or to move away from it, and say, go to a mechanism where we explicitly register hooks. We're adding complexity, and on top of that, we have to patch things together with hacks like this ...)

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs review

How about storing the mapping as it is, i.e. "node_token() was defined in token.module"? I'd like to understand why that isn't possible.

This is exactly what happens today, and exactly why this issue is opened. Because the registry stores "node_token() was defined in the token.module", node_token() cannot be recognized as a implementation of hook_token() by the node module. That's IMO, a good thing.

My stand on #86 is still the same:

  • Having everything in a flat namespace is horribly broken, as already described in #367355: Module names should not contain an underscore: the Views module defines views_preprocess_comment(). Everything is alright, until someone wants to create the views_preprocess module. Once this module is enabled, that function magically becomes and implementation of hook_comment().
  • We don't want to facilitate the job of modules that want to implement hooks on other modules behalf. Even if it's unavoidable, it should not be easy: we want module maintainers to collaborate with each other, at least in the long term.
Anonymous’s picture

The description of the 5 solutions in #64 was tremendously helpful, but ... why can't the registry remember that node_token() was defined in token.module? It sounds to me like the registry's storage model is broken, and that we're trying to work around that in complex ways. How about storing the mapping as it is, i.e. "node_token() was defined in token.module"? I'd like to understand why that isn't possible.

this comment makes me wonder about the registry a whole. it betrays a misunderstanding of how the registry works right now, which damien corrected in #88. i've seen this from other core hackers - the registry is becoming another section of drupal that very few of us understand at any given point in time.

trying again to get back to what i think is the main issue - are we doing to make it possible in drupal 7 to know without any guessing which functions are hook implementations, or are we going to keep the drupal 6 status quo and go with guesses?

if the latter, then we can get away with a fairly simple hack, and maintain the situation where the module system can call functions that weren't intended to be hooks as hooks.

if the former, what mechanism do we want to build in to core to let module authors signal which of their functions are hook implementations?

Crell’s picture

I will disagree with Damien on one point; we do want to make it easy for modules to implement hooks on another module's behalf. There are a number of modules where for various reasons the views integration, say, is provided in a sub-module. Why make that needlessly hard?

I am OK with a "guess with override" mechanism. The guessing is, currently, fairly good (surprisingly so given how many modules we have), but as I said before we do want to leave ourselves an "out" in case of serious collision.

The only question to me is whether the "out" left in chx's #86 is the right one. Let's focus on that.

cburschka’s picture

We don't want to facilitate the job of modules that want to implement hooks on other modules behalf. Even if it's unavoidable, it should not be easy: we want module maintainers to collaborate with each other, at least in the long term.

But core will never implement contrib hooks. Even among different contrib modules, hook implementations are rarely exchanged outside of strict dependencies, because the maintainers don't want to chase APIs that aren't under the rigorous control of core. Even in the long run, the majority of contrib-defined hooks will be implemented by the module that defines them.

cburschka’s picture

implemented by the module that defines them

Addendum: That leads to an interesting idea - make it easy for hook-defining modules to implement the hook, and leave out everyone else. Ideally, this would be accompanied by an initiative forcing contrib-defined hooks to be prefixed by the base name, as in "hook_definer" and implementations as "implementor_ definer" (which could reside only in implementor or definer, but nowhere else). But the caveat here is that it would leave glue modules in the dust when they try to bridge two modules that otherwise ignore each other.

All in all, I would argue that allowing hooks and implementations to roam free is exactly what makes the modular system so powerful. If we didn't use that, we could scrap the magic function naming entirely and move to classes or singletons (which I don't think should happen).

chx’s picture

All in all, I would argue that allowing hooks and implementations to roam free is exactly what makes the modular system so powerful. If we didn't use that, we could scrap the magic function naming entirely and move to classes or singletons (which I don't think should happen).

Right. Now, let's get back to #90 : The only question to me is whether the "out" left in chx's #86 is the right one. Let's focus on that.

Anonymous’s picture

i'm ok with what crell indicated as a way forward. as for the implementation, i'd prefer modules returned a data structure that the registry used, rather than inviting modules to hit the registry table directly. pseudocode:

foreach (module_invoke_all('registry_rebuild') as $resource_name => $data) {
  db_merge('registry')
    ->key(array('name' => $resource_name, 'type' => 'function'))
    ->fields($data)
    ->execute();
}
Crell’s picture

I would also favor a declarative info hook (with an alter, for good measure) over "there's the table, have fun", as the former is much more consistent with the direction newer APIs are heading. It also makes extending to per-hook weights later very easy.

chx’s picture

Later is later. There is still no use case for that hook so I am against code that tries to cover a case that might very well not exist. Per hook weights, in due time. Also I would not want this issue to solve the "hooks called registry rebuild" issue which is a valid issue. My hook is called after registry is rebuilt.

catch’s picture

Priority: Normal » Critical

Bumping to critical since this is going to stop modules like Views being fully ported to HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

do we still need this? now that #449198: SimpleTest: Clean up test loading and related API has landed (which i still think is entirely the wrong approach, but whatever), if modules that want to implement a hook for other modules do it on a file-by-file basis, then we already have them covered.

chx’s picture

I still think autodetect would be fun but the issue has turned into a clusterfuck. It's fantastic how a good idea can be derailed by cases that do not even exist. I will try to get the people who followed up follow up again but I am afraid that Drupal 7 wont get back autodetection and I will point here people who complain because they will.

Crell’s picture

If Drupal 7 doesn't have a solution to this problem it is DOA. I think we said above that autodetect-with-manual-override is acceptable.

Anonymous’s picture

Crell: i agree about needing this, but HEAD right now provides a hook for modules to change the what files will be parsed by the registry, and what module will be associated with the file, so they can achieve the goal right now.

if people are ok with the restriction that you can't mix hooks you implement for other modules in the same file (so, you need to put all the hooks you implement for node module in mymodule.node.hooks or similar) then all we need is to update the documentation.

as i said before, i think the file-based alter is the wrong way to go, but its in now, so i'm wondering if introducing another way to alter the registry on top of the existing one is a good idea.

Crell’s picture

I only barely comprehend the current alter from your description. If I can't follow it, most developers won't have a clue. :-) Especially as we're making things more manual that way than the current automagic method.

boombatower’s picture

Just to clarify the hook that made it in solves a separate use case, but can be used to solve this one in a restrictive manor as has been mentioned. Once a solution for this issue is agreed on we can make whatever changes to the existing hook that make sense or even remove it.

The problem is this issue did not deal with my original use-case _AT_ALL_. That being that SimpleTest needs to be able to add files for disabled modules so that it can take advantage of the registry. The issue being discussed here is how to implement a hook on behalf of another module (and assumes the code in question is contained by an enabled module).

Rather then hold up all my SimpleTest improvements that depended on that patch it seemed logical and I had approval.

Just to make it clear that hook was not intended to be the solution and just ignore all the discussion here. Please feel free to make a patch that solves this problem however we agree and alter the hook/remove it completely. Just make sure SimpleTest works when everything is said and done.

chx’s picture

I am happy to revive the patch if DamZ and Crell and justinrandell agree that a) we want magic renaming b) we want a hook to fire after registry rebuilt (its not like it costs anything). Please stop discussing and post a) yes b) yes to make it clear.

chx’s picture

Sigh.

We want

a) magic guessing
b) fire a hook at the end of registry rebuild which just signals the end of registry rebuild (kinda like nodeapi and update signals similarly).

Crell’s picture

After some discussion with chx in IRC, I am +1 on magic-detection with an "after build" style hook. If we find that it's actually getting used, we will add proper API functions to manipulate the registry later.

Anonymous’s picture

@chx: i'm ok with a) and b) using something like the pseudocode in #94.

@boombatower: thanks for clarifying. i think the best approach would have been to make the registry parse all files for all modules regardless of their installed/uninstalled state, and track enabled/disabled and other metadata on top of that. the registry already has 'if it didn't change, we don't need to reparse it' logic, so this wouldn't be expensive.

this would make it easy to add simple registry function that provide information about uninstalled modules. also, if you think about the registry pre-load hacks in simpletest, that becomes very easy - you just reload all of the underlying registry info into the test side database, and setUp simply updates the metadata. that would be quick, and would make it easier to avoid the issues we currently have with preloading the registry.

chx’s picture

justin, WHY do you want that API? It's a waste of code, noone ever is going to use this. IF someone comes up with a usage, we can add an API.

Anonymous’s picture

chx, what? when i suggested the change-the-registry-db-entries-directly approach, you replied in #35 with "Ouch! Encapsulation gone. Why would any module operate directly on a db table? Thats not nice...". i agree with what you said there.

we have a use case for contrib modules wanting to alter the registry, so do we just send them a signal and let them operate directly, or do we give them an API that doesn't require that? i'm saying an API is better, but i'm kind of over it at this point, and if Crell is ok with just sending the signal, i'll defer.

chx’s picture

we have a use case for contrib modules wanting to alter the registry

Pray tell me what use case? This is what I am asking for like fifty followups now. When I said encapsulation gone we were not yet thinking on magic naming. To recap: if you have function foo_bar_baz and both foo and foo_bar modules enabled then the system will say "foo_bar_baz belongs to foo_bar module and should fire on hook_baz". The only reason we need something after the rebuild is if you want "foo_bar_baz belongs to foo module and should fire on hook_bar_baz" but, as we discussed until our noses bleed this is a very very unlikely case which has a good chance to be broken regardless of the registry so -- I am happy you defer :)

Damien Tournoud’s picture

I'm a big -1 on reintroducing any form of magic naming.

Now that we have hook_registry_files_alter(), modules like Views could simply alter the registry to make it believe that node.inc belongs to the node module.

Damien Tournoud’s picture

I described in #88 how magic naming is a problem.

The Views module has a views_preprocess_comment() function. As soon as you enable an hypothetical "views_preprocess" module, this function is automatically transformed into an implementation of hook_comment(). That's very wrong.

Of course, even without magic naming, "views_preprocess" couldn't implement the hook_comment() hook, because it would trigger a fatal error. But at least, the Views module should not be affected by the fact that any views_* module is enabled. It's a basic issue of separation of concerns.

Reintroducing magic naming basically mean that you cannot define any 'views_*' module, because it would requalify the meaning of any function defined by the views module.

chx’s picture

Damien, it is not magic naming that introduces issues. If you create a views_preprocess module when views module alreayd has views_preprocess_comment function then you get a fatal error when you implement hook_comment in views_preprocess. This is a fundamental problem in our hook system. If you inconvinience certain module authors to make the creation of such modules easier then you paved the way to hell. It's not the registry's problem that PHP has one flat namespace.

Damien Tournoud’s picture

All those issues come from the fact that our function naming is imprecise. The only true solution for this problem, is to move to new naming convention, for example <module name>__<hook name>().

I'll be all for magic naming if we do that. But only if we do that.

chx’s picture

A new naming convention is a whole separate issue. In this issue, the magic naming is the best for the subject IMO.

catch’s picture

hooks already use magic naming. I thought that was the point of them, and don't see why leaving them hamstrung in D7 would be anything other than a regression.

sun’s picture

FWIW, I repeat my support for the magic discovery approach.

We literally ran in circles here and ended up identifying again that our hook namespaces won't allow for the mentioned edge-case at all. The registry can't solve the PHP fatal errors triggered by two modules that happen to implement the same function name. That not only applies to PHP functions and classes, but also to JavaScript. This real world limitation hit me just recently, when users installed http://drupal.org/project/admin_menu module concurrently with http://drupal.org/project/admin module.

We can discuss to change our hook name pattern, but that is definitely a separate issue.

chx’s picture

Status: Needs work » Needs review
FileSize
7.06 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

FileSize
8.17 KB

updated patch to fix test failures after chx asked me to have a look in #drupal. only change is to not run the preg_match to guess the module if $module_preg is empty.

Anonymous’s picture

Status: Needs work » Needs review

woops, needs review.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, I intended to that -- I deem this RTBC after the many, many go merry-go-around rounds.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

eaton’s picture

After reading through the entire thread I'm still a little baffled on the proper approach to this, both before and after the patch.

My understanding is now that module_invoke_all() now enforces a degree of 'pseudo-namespacing' because it will not load an include file from 'token' module in order to execute a hook for 'node' module. Is that correct?

And this patch, its purpose is to allow modules to implement hook_registry_files_alter() and say, 'This include I have ... it should actually be treated as part of node.module...' Is that correct?

chx’s picture

Status: Needs work » Needs review

retest.

Damien Tournoud’s picture

@eaton: no, you can already implement hook_registry_files_alter() and tell the registry "hey, please consider that this token.node.inc file belongs to the node module". We tested that with Gabor when doing the port of Form Builder to D7, and it works flawlessly.

I'm unsure about the purpose of this patch.

eaton’s picture

Right. Just for reference, here's the snippet being used in the token port for d7:

function token_registry_files_alter(&$files, $modules) {
  $dir = $modules['token']->dir;
  foreach ($modules['token']->info['files'] as $file) {
    $path = split('/', $file);
    $inc_file = array_pop($path);
    $pieces = explode('.', $inc_file, 2);
    if ($pieces[1] == 'tokens.inc') {
      $files["$dir/$file"]['module'] = $pieces[0];
    }
  }
}

It walks through files in /token/modules and re-assigns them based on their naming patterns. node.tokens.inc gets parented to node module, etc. Works well.

Crell’s picture

I believe the goal is to not have to do that for every module that implements a hook on behalf of another module, as that would be, um, a lot for something like Views. Adding hoops to jump through for what we are able to do now in D6 without effort is a bad thing.

merlinofchaos’s picture

To be fair, I think Views would be just as happy to use that same code. It already keeps everything but one hook in MODULENAME.views.inc anyway, and I realize that one hook does not have to be elsewhere in D7 because Views will no longer be including the .views.inc files anyway.

So I actually like eaton's solution.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I am RTBCing because of sun's support and because tho eaton's code works of course, it's not necessary. The patch above passed the retest.

Pasqualle’s picture

Priority: Critical » Normal

#97 <-> #130

Anonymous’s picture

<sarcasm>woo, this has been such a fun issue</sarcasm>. to summarise:

1. we already have a solution for modules that want to implement hooks on behalf of other modules, but it requires code like that posted by eaton in #128.

2. this patch changes the way hook and module guesses are made. currently, we guess if a function is a hook implementation by checking to see if a file's functions start with the name of the module that defined the file. this patch makes the check much broader - we look to see if the function starts with any enabled modules' name - if it does, then we guess the function is a hook implementation for the matching module. in many cases, this should eliminate the need for the type of code listed in #128.

3. this patch also adds a signal-hook that lets any interested module know that a registry rebuild has been completed. any module that can't get what it needs from 1. and 2. can fix things at this point.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Pasqualle’s picture

question: If I modify the code in comment #128 like:

$files["$dir/$file"]['module'] = '_token_' . $pieces[0];

then for example in poll.token.inc I can use

/**
 * Implement hook_tokens().
 */
function _token_poll_tokens

this way if the poll module wants to implement hook_tokens() it still can use the poll_tokens() function. Would this work? I am asking because I need to implement a hook in a contrib module on a behalf of another contrib module, and I do not want to take its namespace.

catch’s picture

Status: Needs work » Closed (works as designed)

No more registry so we're back to Drupal 6 in terms of how things work.