Very elegantly done avoiding another function (should it now be toggle_hooks? ).

It certainly was worth the wait.

Synched and committed in my repo as well.

Janusz

Janusz

/*
    Non active hooks are not included in $Hooks array, so we can use special function to 
    activate.
*/

Only, in this case, at this point of entry into the hooks_invoke even the active hooks are not in it! There are a host of functions that are outside the hooks class definition that create a new instance of the hooks class and then invoke the hooks methods!

Install the extension in the default company and activate it for a non default company alone and then try to de-activate it and see if the tables get dropped.

Only the official dashboard extension has the drop sql in it.

Try to only install the dashboard extension (and not the theme) and then activate it and then de-activate it and you will see that the tables remain. At first I thought that it was to save the tables for a re-installation so that user changes are persistent but then most extensions drop the table if it exists before re-creating them during the activation phase and hence it gives us an illusion of having been deleted during deactivation unless the tables are checked just after deactivation.

The usage of XOR is may be standard even if not very widely used, but the usage of the hook_invoke by callback isn't where a simple if...else could have sufficed.

The real solution however, would be to make sure that the global $Hooks array gets pupulated earlier than this invocation and does not get overwritten thereafter. In fact, we have @kvvaradha to thank as this was investigated during the vetting of his hrm extension based on the dashboard skeleton.

It is pertinent to note that solution may lie in the fact that the extension install and uninstall works perfectly in includes/packages.inc wherein the $Hooks array gets populated with the active extensions in:

            if (($ext['active'] == true) && file_exists($path_to_root.'/'.$ext['path'].'/hooks.php'))
            {
                // we need to include the new hooks file to activate extension
                include_once($path_to_root.'/'.$ext['path'].'/hooks.php');
                foreach($db_connections as $comp => $db)
                    activate_hooks($ext['package'], $comp);
            }

Perhaps something like this should precede the hooks_invoke in admin/inst_module.php.

NB The XOR usage in above code is rather standard - this is just simple test which is true if and only if  'active' flag changes.
Janusz

Lines 213 to 225 of the current admin/inst_module.php file are:

    foreach($exts as $i => $ext) {
        if ($ext['package'] && ($ext['active'] ^ check_value('Active'.$i))) {
            if (!$ext['active'])
                $activated = activate_hooks($ext['package'], $comp);
            else
                $activated = hook_invoke($ext['package'], check_value('Active'.$i) ?
                 'activate_extension':'deactivate_extension', $comp, false);
            if ($activated !== null)
                $result &= $activated;
            if ($activated || ($activated === null))
                $exts[$i]['active'] = check_value('Active'.$i);
        }
    }

The second line (first if) above has an XOR (^) which means that the code fragment is executed only if either

1. the extension was already active or

2. the activate extension checkbox has just been ticked and submitted

but not both.

The second "if" activates the extension using the direct function activate _hooks() if it was inactive which because of the XOR condition above would mean that the activate extension checkbox had just been ticked. This works correctly and the module gets installed. However, this does not happen through the callback portion of the hook_invoke() function through which it is never called. The use of the direct function activate _hooks() was necessary since the value of the $Hooks global array variable is empty when it enters the activate_extension() method by callback if the hook_invoke() was used.

The "else" of the second "if" triggers the callback only for the deactivate_extension() method when the activate extension checkbox has just been unticked whilst the extension was active. Here, the value of the $Hooks global array variable is empty when it enters the said deactivate_extension() method which exits without executing the deactivation sql if any. It is for this reason that the new direct function deactivate_hooks() was used to do the job.

The esoteric callback method in hook_invoke() seems to make the code quite a bit of a challenge for the average user to comprehend and I was initially blinded to conclude that the said deactivate_extension() method was never called. We should try to use simple and easy to understand constructs for ease of debugging / troubleshooting by the average user for mass trusted adoption.

Steps to reproduce:
1.  Install and activate for one company any extension (currently, only dashboard extension has a drop sql separately) that creates tables and has a means to drop them as well. Alternatively create an extension that only creates one table and then on deactivation drops that table and then install and activate it for one company.
2. Now choose to deactivate it for the company for which it is currently active.
3. Check and find that the newly created table(s) have not been dropped despite deactivation.
4. Use my new function method and then repeat the steps above and you will find successful deactivation with the newly created table(s) dropped.

Well, in your patch you have removed the line where the function was used indirectly by hook_invoke method (the method name was passed to be executed in this function) as callback to drop the spare tables. Could you explain what was the reason for this?

Janusz

I am out of office right now, but will address him when I am back.

Joe

In case you are testing with PHP 5.6+, please see what functions are deprecated / dropped in 5.4, 5.5 and then 5.6 and make sure you make appropriate changes / replacements.

There is no intention of the devs and myself in supporting any later versions of PHP other than 5.3 for FA v2.3.x. You are pretty much on your own here.

To assist others however, you may post your findings in a separate thread for such later PHP versions. Such findings posted in this thread will only serve to confuse end users - yes, it will make for big consultancy business...... but FA has been designed with simplicity in mind and for self service.

The code portion for activating extensions has not been touched in this fix and if it doesn't work for you now, then it never did in the first place prior to this fix as well.

There was a missing function deactivate_hooks() which has now been added.