-
-
Notifications
You must be signed in to change notification settings - Fork 823
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
Add Joomla 4 compatibility to CRM_Utils_System_Joomla::loadUser() #31601
base: master
Are you sure you want to change the base?
Add Joomla 4 compatibility to CRM_Utils_System_Joomla::loadUser() #31601
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
// we need this because only the specific site application object has the login() method | ||
// the below is adapted from <JOOMLA_ROOT>/includes/app.php on Joomla 4.4.9 | ||
$joomlaBase = self::getBasePath(); | ||
require_once $joomlaBase . '/plugins/authentication/joomla/src/Extension/Joomla.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@webmaster-cses-org-uk I'm thinking this is probably sensible in a cron.php
context but does it still make sense if Joomla has already been booted does it cause any issues you think?
@demeritcowboy @totten you folks might now if we do require_once on files that might have already been included by joomla would there be warnings/issues emitted by PHP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see how it would give a warning.
Regarding the timing of it I don't know enough (anything) about joomla to comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seamuslee001 I guess we shouldn't really be calling cron.php
if Joomla is already booted but we could put some sort of condition round it to guard against double-initialisation (e.g. check for _JEXEC
defined already or something like that). What I would say is this whole file is going to require a major rewrite for Joomla 5, all I've done here is a quick fix, but I could look at putting an extra condition round lines 499-512.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry forgot to mention... I did check and as far as I could tell this method is only called from cron.php
, but that's not a reason/excuse not to make it robust like you suggest!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK 3rd reply then I must leave this for a bit!
Having had a closer look, there is already some similar code in loadBootStrap()
in the same file, which looks like it may be called before loadUser()
in cli.class.php
. However that loads either the ConsoleApplication
(which does not have the login()
method - hence why cli.php
fails) or the AdministratorApplication
(which does have login()
but for whatever reason we seem to want the site
here - maybe this is wrong / not necessary?).
So with this change we actually would be overwriting the initialisation done in loadBootStrap()
- although it seems to work fine - so I would suggest that the actual fix is to harmonise these somehow so that the correct application object is available when needed.
Overview
This PR proposes a fix to the issue https://lab.civicrm.org/dev/core/-/issues/5645, where it was found that cron jobs running via cli.php are not compatible with Joomla 4.
Before
Running a cron job on Joomla 4 results in process failure with the following error message:
After
Cron job completes successfully.
Technical Details
This has been tested using CiviCRM 5.78.3 on Joomla 4.4.9. The proposed code change is based heavily on the suggestion made in this discussion (https://groups.google.com/g/joomla-dev-general/c/55J2s9hhMxA?pli=1), which replicates the object instantiation code used in Joomla's
includes/app.php
. It then needed some furtherrequire_once
directives to get this to work.Comments
This is ONLY a workaround / temporary fix, as it does not address the wider Joomla 4/5/6 compatibility issues such as using deprecated naming conventions and methods. Furthermore, the need for
alias()
is a temporary bridging arrangement during the transitions between Joomla 3 and 5 (possibly 6).As such, this issue will need to be revisited later. This will still NOT work with Joomla 5 or above.