This is a great experiment. Can't wait to see how it unfolds. Thank you for working on it!

One minor suggestion, though, if you don't mind:

Looks like the C extension declares functions with the same name as the PHP implementation.

This is probably a bad idea. Such approach will always require Core to be hacked. If instead you name your functions in some generic non-Drupal-specific/duplication way, potentially

a) A PHP distribution may accept these functions eventually
b) The PHP implementation in the next Drupal core could check for the existence of the extension and delegate to them seamlessly, short-circuiting PHP code. No core hack required.

However if the function names collide this won't be possible.

Comments

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Closed (works as designed)

I think the point *is* to replace the specific core functions, therefore some hacking will be necessary.

irakli’s picture

irakli commented
Status: Closed (works as designed) » Active

If you read the issue description a little more carefully - you will see that it does not have to be.

I would ask next time to read the issue a little more carefully before closing it. The question was clearly addressed to the maintainer of the module.

nicholasthompson’s picture

nicholasthompson
Primary language English
commented
Version: » 7.x-1.x-dev

If this module becomes accepted, though, colliding function names can be solved by core just wrapping these functions in if (function_exists(...)) { ... }.

Or, move said functions into an include which can be switched in your settings.php (like cache.inc)...

dhthwy’s picture

dhthwy commented

Damien Tournoud first brought this up on IRC and made a good suggestion on a possible solution for this.

What we'll be doing is renaming the PHP functions in the extension to drupal_static_ext and check_plain_ext (and any new functions added will follow the same pattern). A new function will be added to the extension that when called will magically update Drupal functions to use their counterpart functions defined in the extension. Eventually as more stuff gets moved into the extension we can add the ability to pass in an array of functions you want the extension to take over.

So with this change instead of commenting out functions you can simply add drupal_ext_activate() to index.php after bootstrap.inc is loaded in: 

define('DRUPAL_ROOT', getcwd());

require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_ext_activate();
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
menu_execute_active_handler();

Does the name drupal_ext_activate make sense for this? I'm not sure exactly what to call it.

dhthwy’s picture

dhthwy commented
Status: Active » Fixed

Committed to HEAD.

Renamed the extension's PHP functions to not conflict with Drupal and added a new function called drupal_extension_enable(). The new function is used to override Drupal functions where the extension implements them. Returns TRUE on success and FALSE on failure. It will only fail if you attempt to call it before the functions it overrides hasn't yet been loaded/evaluated by PHP. A patch is now provided to do this for you.

A parameter will be added to it soon so you can tell it which functions to override at specific points during run time. These will be file-specific overrides. For example, to tell it to override functions the extension implements in includes/bootstrap you'd do: drupal_extension_enable(DRUPAL_EXTENSION_BOOTSTRAP). And for includes/theme.inc, it would be drupal_extension_enable(DRUPAL_EXTENSION_THEME).

irakli’s picture

irakli commented

@dhthwy,

that sounds great. Thank you.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

donquixote’s picture

donquixote commented
Status: Closed (fixed) » Needs work

What about different drupal versions?
Say, we have a hard-wired version of drupal_static, that is drupal_static_ext().
Then Drupal 8 preview is released, with a slightly different behavior of drupal_static().
Then in Drupal 8 beta and final we see yet another change in the drupal_static() implementation.
Instances of these different Drupal versions might all run in parallel on the same web server.

To be really safe, we need to introduce a naming scheme that does distinguish Drupal version. Such as
drupal_static_ext_8_13.
This would be for all Drupal versions from 8.13 upwards, unless a higher number implementation is found.