CRM-20904 : joomla cron run - load all daos defined in extensions

[jitendrapurohit]

Joomla doesn't invoke any hooks until J_EXEC is set.

And when cron job is executed via URL - Config run loads all core DAO's in $self::entityTypes before joomla is actually bootstraped, this value is unchanged and only keeps core value defined in it.

Fix here does a fresh load of all the DAO's defined in extensions, custom modules, etc soon after the bootstrap is done successfully.

---

• CRM-20904: Joomla - Fatal Error when schedule job is executed from cron.php

[jitendrapurohit]

jenkins, retest this please

[totten]

This is interesting.

If you said there was a bug with Joomla extensions where the cache in AllCoreTables got stale, that would make sense. (IIRC, hook_civicrm_entityTypes is actually a boot-critical hook -- the first hook fired. This would mean it fires before Civi has a chance to bootstrap Joomla, so it would initialize without the benefit of any CMS modules.)

Gonna see if I can replicate...

[totten]

I've tried to reproduce the original issue (before patch). The steps taken were:

1. Go to my local Joomla build. (This was running v4.7.21; created via civihydra. On PHP 5.6.30, mysql 5.7.18, and Ubuntu 16.10.)
2. Clone https://github.com/eileenmcnaughton/nz.co.fuzion.accountsync into build/hydra-joomla/administrator/components/com_civicrm/civicrm/ext/nz.co.fuzion.accountsync
3. Install. (This failed at first, but I hacked sql/auto_install.mysql to work-around.)
4. Add a print statement to one of the jobs (api/v3/Job.php -- civicrm_api3_job_update_greeting) so that we'd be able to see if it runs. ( printf("<pre>%s %s %s</pre>", __FILE__, __LINE__, var_export(CRM_Core_DAO_AllCoreTables::get(), 1));)
5. In a browser, open http://hydra-joomla.l/administrator/components/com_civicrm/civicrm/bin/cron.php?name=admin&pass=topsecret&key=abcdabcdabcdabcd&job=update_greeting&ct=Individual&gt=email_greeting
6. Observe that the job executes and runs the print statement.
7. Observe that the output does not include AccountContact

But I haven't been able to reproduce. :( I have reproduced.

(Edit: using var_dump() to inspect seems problematic because it truncates output. Changed to use var_export().)

[jitendrapurohit]

Thanks @totten

Worth seeing if we get all entityTypes defined by hooks of accountsync extension? Or if we just keep a print statement in this hook and see if it fires from cron.php (when it loads all entityTypes from AllCoreTables.php)?

[totten]

OK, I get it now. IMHO, the problem is really with CRM_Utils_Hook_Joomla bailing out. It makes sense to skip stuff like $app->triggerEvent(), but we should fire the extension hooks (commonInvoke).

Why? The issue affects all "boot-critical" hooks fired by Civi extensions -- i.e. hook_civicrm_entityTypes, hook_civicrm_container, hook_civicrm_config. The current patch in 10692 is narrowly tailored to clear cache for entityTypes, but there's no cache you can clear that will retroactively fixup the container. (Even if we put aside performance concerns and tried... the container is written to CachedCiviContainer.*.php, and the class-definition becomes immutable within the page-request.)

I think this patch would have the effect of aligning Joomla behavior with Drupal/WordPress behavior:

diff --git a/CRM/Utils/Hook/Joomla.php b/CRM/Utils/Hook/Joomla.php
index fd1793c..4e24264 100644
--- a/CRM/Utils/Hook/Joomla.php
+++ b/CRM/Utils/Hook/Joomla.php
@@ -123,6 +123,12 @@ class CRM_Utils_Hook_Joomla extends CRM_Utils_Hook {
       }
       return $result;
     }
+    else {
+      // CRM-20904: We should still call Civi extension hooks even if Joomla isn't online yet.
+      return $this->commonInvoke($numParams,
+        $arg1, $arg2, $arg3, $arg4, $arg5, $arg6,
+        $fnSuffix, 'joomla');
+    }
   }
 
 }

[jitendrapurohit]

yes, thought about this but proceeded with the simple/safe approach of updating the static variable self::$entityTypes which was not filled with extension DAOs. I'll test this on the affected site and update here. Thanks @totten

[jitendrapurohit]

Tested the above diff on the affected site and it worked well in calling the extension hooks. I've updated the PR with the same. Thanks!

[eileenmcnaughton]

@totten have your issues on this been addressed?

[totten]

Thanks for reviving!

I'd prefer to merge with just 3c27459 (not 88d3c0d). To my reading, 88d3c0d became unnecessary and makes the semantics less consistent across UF's.

[jitendrapurohit]

whoops, second commit was a partial update of this PR - thought I removed the original changes. I've updated it now.

Thanks @eileenmcnaughton @totten

[eileenmcnaughton]

@totten looks like this has ben adjusted to meet your requirements - can you recheck & hopefully merge

[eileenmcnaughton]

I'm merging this as it seems fairly clear that the eventual patch was suggested by @totten (the reviewer) and tested by @jitendrapurohit & used to replace the original. I think Jitendra becomes the tester & - it's too confusing - but I think the final result represents a code collaboration & has been tested