A frequently requested functionality is the ability to override or extend core module functions.

This small patch is an initial take on how to enable function extension and overriding. It takes the existing module_invoke() function and adds tests for override and extension functions, in the forms modulename_originalfunction_override() and modulename_originalfunction_extension(). So, for example, an override function for the core taxonomy_node_form() function as defined by a module named testmodule would be called testmodule_taxonomy_node_form_override(), and would be run instead of taxonomy_node_form(). And testmodule_taxonomy_node_form_extension() would be run every time taxonomy_node_form() was called, hence "extending" it.

Of course, this approach would only be useful if we converted most - or all! - of our current module function calls to use module_invoke()...

Comments

Stefan Nagtegaal’s picture

I'm not sure I get this..

Am I right when I think we can override (and change) any function in drupal with this patch, which would give us the possibility to:
- override forms;
- override functions which generates the node links, so they (finally) could be hidden instead of always displayed;
- any other advantages which i'm missing atm? (probably a lot, but can't think of any right now, right here)

Stefan

drumm’s picture

-1

This will just encourage hacks and create weird bugs as the code interactions become more complex.

The example form will be themeable when the new form API is in place.

nedjo’s picture

Thanks for the comments.

I'm throwing this out not because I think the solution I've suggested is full or even sound, but to try to stimulate a better solution.

(The example I gave was random and not designed as one particularly needing overriding or extending.)

Basically, the problem is: because we don't use PHP's class object functionality or some substitute, we don't have a way of overriding or extending object methods -- in Drupal, core functions -- beyond the limited functionality explicitly exposed through hooks. This problem has fairly significant impacts.

Consider the taxonomy system and contributed modules. We've seen various approaches to enhancing taxonomy display, e.g., taxonomy_menu. But, because they can't (readily) add to the existing (core) taxonomy system, they tend to bypass and replace it, e.g., by creating new urls and displays. So we get a variety of competing approaches, each implementing its own limited set of functionality, often replicating code. As site admins, we have to choose between one or another implementation, rather than being able to seamlessly combine them all.

Ideally, we'd to be able to add just the specific enhancement we want.

Is there another, better way to emulate object method overrides and extension? Or should we simply accept this limitation and work on improving extendability through hooks and theming?

chx’s picture

StatusFileSize
new1.42 KB

I think this method would be a better way to extend pretty much anything. Based on $_GET['q'] and the arguments from theme() you can determine which theme function called you and can add anything. This won't allow override but would allow extension. If we want override then simply replace both hooks with one at the end and use $function() syntax to enable references.

chx’s picture

StatusFileSize
new1.48 KB

Corrected.

jose reyero’s picture

Hey, I'd like very much both ideas, nedjo's and chx's. One for module calls and the other for themes, but I think they're basically the same idea, and for sure it would save me the need for a lot of patches, and maintaining patched versions of Drupal would be much easier.

But I'm thinking of this more simplified implementation:

Instead of having overrides as module hooks, we could allow for generic 'override' functions, to be placed anywhere in the code. This way I could have them in a site's config file or even in a specific module for a site. My idea is not using them in generic modules but instead, using them as some "patching" mechanism.

I'm talking of having somewhere a function like override_taxonomy_node_form() or override_theme_table(). Having plain functions instead of hooks could save some time at run time.

In case of module_invoke, you dont need to have override and extend. The overriding one can call the original function (or not) and then add stuff if needed.

Also for theme functions, you don't need pre and post. It would be enough to pass the result of the original theme function, to add html before or after, or maybe the overriding code can take care of calling the original theme funcion if needed.

Hope my comments are some help and please: keep working on this!

I like patch bingo :-)

Jaza’s picture

I agree with Jose: being able to override function modulefoo_functionbar() by writing a function override_modulefoo_functionbar() is definitely the way to go. And it wouldn't be too hard to implement either, since we already have module_invoke() and module_invoke_all() set up to handle all function calls (we'd just need to enforce the use of these functions more strongly).

What's more, I think that a system like this would be quite consistent with the current theme override system that we have (i.e. phptemplate_table() can override theme_table() etc). Because Drupal lacks the benefits of a system that uses objects and classes, there is a definite need for a simple and effective way to extend a module.

robertdouglass’s picture

-1. This sounds tempting like a pot of honey. I think it is not a great idea, however. One has to ask how far we want to go in hacking in expensive object orientation functionality (polymorphism) before we just decide that using some classes here and there is a better idea.

For the sake of poor-mans-polymorphism we lose all encapsulation.

Why not put a copy of the module with the alternate function in your site's folder in the sites folder? That achieves the same, doesn't it?

Where are the benchmarks that show just how much processing time this takes when a site has lots of modules installed?

How will this affect the memory profile if both original and overriding versions of a function have to be loaded into memory?

killes@www.drop.org’s picture

-1 as long as no benchmarks are provided.

jose reyero’s picture

Status: Needs review » Needs work

robertDouglass:
> One has to ask how far we want to go in hacking in expensive object orientation functionality (polymorphism) before we just decide that using some classes here and there is a better idea.

You're right, but actually I'm for OOP too

> For the sake of poor-mans-polymorphism we lose all encapsulation.

Well, I was thinking here only of "cheap patching"

I think that "poor-mans-polymorphism" is already implemented all through Drupal.

> Why not put a copy of the module with the alternate function in your site's folder in the sites folder? That achieves the same, doesn't it?

You are talking about a patched module file, which is what we are trying to avoid.

>Where are the benchmarks that show just how much processing time this takes when a site has lots of modules installed?
>How will this affect the memory profile if both original and overriding versions of a function have to be loaded into memory?

If implementing as a new hook, yes, maybe benchmarks are needed. But if it's only a single funcion check, I dont think we need that

nedjo’s picture

Status: Needs work » Needs review
StatusFileSize
new1.04 KB

Here's a two-line patch implementing Jose's idea of function override_modulename_functionname().

Jaza’s picture

> Why not put a copy of the module with the alternate function in your site's folder in the sites folder? That achieves the same, doesn't it?

What you are suggesting is equivalent to telling theme developers:

We're removing the theme() function from Drupal core. If you want to override any theme functions from now on, please put a copy of theme.inc in your 'sites' folder, and modify that copy.

Surely that analogy speaks for itself: module patching is currently a maintenance nightmare, and this addition will make the nightmare significantly more pleasant.

jose reyero’s picture

+1 for nedjo's patch, which looks quite straight forward and powerful.

robertdouglass’s picture

Doesn't this latest patch allow for namespace conflicts? I thought $modulename had to be part of the function name.

robertdouglass’s picture

The only way I see to resolve the potential namespace conflict that you're introducing with this patch would be to do what the theme system does now; decide on a per-function basis upfront which function version is going to be used, and it would have to do it by loading all of the modules and making a map of where the various potential overrides exist, and then make an interface so that the admin could choose which one is to be used. In other words, you'd be doing exactly what Java or PHP5 does when you code this:


object = new SomeClass();
object.callMethod();

Here's a better suggestion; why don't we require that any functionality that needs to be patched or overridden exist as a class?

moshe weitzman’s picture

Component: module system » base system

don't you get virtually eliminate all namespace issues of you include the full function name that you are overriding? for example, override_image_insert() or override_taxonomy_select_nodes()

killes@www.drop.org’s picture

Status: Needs review » Closed (won't fix)

I don't see this as a great idea. If you really want to not call the original function you can write your own and replace the callback function in the menu handler be running your module after all other modules. Alternativly use the form API overrides for validate and submit in a similar manner.

If you think this isn't fine grained enough, you should start supplying patches to split up bit callback functions.

I "won't fix" this.

nedjo’s picture

Agreed.

gopagoni’s picture

Version: x.y.z » 5.7
Component: base system » other
Assigned: Unassigned » gopagoni
Priority: Normal » Critical
Status: Closed (won't fix) » Postponed (maintainer needs more info)

Hi
This is Narsing.I'am new to drupal.can u please clearly give the usage of this patch.I have the same requirement in my project.I added this patch into module.inc and i tryed to override faq settings form like this
function cust_taxonomy_faq_general_settings_form_override(){
return "HELLO";
}
cust_taxonomy is my custom module i just tested to override the faq settings form but it is not reflecting.
please help me.

damien tournoud’s picture

Component: other » base system
Assigned: gopagoni » Unassigned
Priority: Critical » Normal
Status: Postponed (maintainer needs more info) » Closed (won't fix)

Drupal offers a lot of mechanisms to extend and alter the behavior of subsystems (Form Alteration, Menu Alteration, Themes, etc.). You really can't have the "same requirement", because that requirement makes no sense. Please read the module developers' handbook (http://drupal.org/node/508) for more information about how your module can interact with the core and with other modules.

And fixed vandalism.

frank ralf’s picture

I have sinned too (http://drupal.org/node/347659) and I wish there was an "official" way of overriding core functions. (Or maybe I just haven't found it.)

Perhaps the solution could be a more object oriented approach for programming those core functions
(http://hudzilla.org/phpwiki/index.php?title=Overriding_functions_in_subc...)?

Kind regards,
Frank

r21’s picture

Assigned: Unassigned » r21
Category: feature » support

Please let me know where to include the patch and how to include in our custom module to override the core functionlity of core module. for reference use this url "http://drupal.org/files/issues/module-extension-and-override.patch" in the module. and its very urgent pls reply as soon as possible

frank ralf’s picture

See http://drupal.org/node/347659 for some pointers on how to cleanly override core functions. Be sure you know what you do!