Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/joomla#41 [REF] Amend Joomla Bootstrapping code and hook code to … #24796

Merged
merged 1 commit into from Oct 27, 2022

Conversation

seamuslee001
Copy link
Contributor

…get cron to work on Joomla 4

Overview

This Alters the joomla bootstrapping process in CiviCRM when we need to bootstrap joomla after bootstrapping civi (e.g. cron.php and cv) to work with Joomla 4 and I think will work with Joomla 3

Before

Cron does not work on Joomla 4

After

Cron I believe works on Joomla 4

ping @monishdeb @vingle @joshgowans

@civibot
Copy link

civibot bot commented Oct 24, 2022

(Standard links)

@civibot civibot bot added the master label Oct 24, 2022
else {
require $joomlaBase . '/libraries/bootstrap.php';
require_once $joomlaBase . '/includes/framework.php';
}
self::getJVersion($joomlaBase);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 can you move this function call to L577 above? That will prevent the internal notices :

Warning: Use of undefined constant JVERSION - assumed 'JVERSION' (this will throw an Error in a future version of PHP) in /Applications/MAMP/htdocs/joomla/administrator/components/com_civicrm/civicrm/CRM/Utils/System/Joomla.php on line 589

Deprecated: Bootstrapping Joomla using the /Applications/MAMP/htdocs/joomla4/libraries/import.legacy.php file is deprecated.  Use /Applications/MAMP/htdocs/joomla4/libraries/bootstrap.php instead. in /Applications/MAMP/htdocs/joomla/libraries/import.legacy.php on line 16

Warning: Use of undefined constant JVERSION - assumed 'JVERSION' (this will throw an Error in a future version of PHP) in /Applications/MAMP/htdocs/joomla4/administrator/components/com_civicrm/civicrm/CRM/Utils/System/Joomla.php on line 593

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb good point updated that now

@@ -74,7 +74,7 @@ public function invokeViaUF(
$app = JCli::getInstance();
}
else {
$app = JApplicationCli::getInstance();
$app = Joomla\CMS\Factory::getApplication();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm not necessary but would be better to have a leading slash, i.e. \Joomla\CMS\Factory::getApplication();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb updated that now

@monishdeb
Copy link
Member

@seamuslee001 thanks for the fix. The patch works for me, tested against Joomla 4 and 3. I have mentioned a few minor changes.

@seamuslee001 seamuslee001 force-pushed the dev_joomla_41 branch 2 times, most recently from e14304d to f00bee2 Compare October 24, 2022 05:37
@monishdeb
Copy link
Member

  • General standards
    • (r-explain) PASS
    • (r-user) PASS Tested the patch in previous J3 and J4, works fine for me.
    • (r-run) PASS Tested on my local, works fine.
  • Developer standards

@monishdeb
Copy link
Member

@vingle can you please test this patch?

@monishdeb monishdeb added the merge ready PR will be merged after a few days if there are no objections label Oct 24, 2022
@monishdeb
Copy link
Member

As per the latest comment here there is an issue with webcron.

@monishdeb monishdeb removed the merge ready PR will be merged after a few days if there are no objections label Oct 25, 2022
@vingle
Copy link
Contributor

vingle commented Oct 26, 2022

Have tested this with an http cron for J3 and J4 - works with both (calling the url returns the right error msgs with the wrong details, runs the job with the correct details).

@seamuslee001
Copy link
Contributor Author

Merging based on @vingle and also Josh's testing in the gitlab

@seamuslee001 seamuslee001 merged commit 2d5bc9b into civicrm:master Oct 27, 2022
@seamuslee001 seamuslee001 deleted the dev_joomla_41 branch October 27, 2022 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants