[dev/joomla#60] Use correct DB client to detect upgrade - #96
Open
webmaster-cses-org-uk wants to merge 1 commit into
Open
[dev/joomla#60] Use correct DB client to detect upgrade#96webmaster-cses-org-uk wants to merge 1 commit into
webmaster-cses-org-uk wants to merge 1 commit into
Conversation
| // if the CiviCRM and CMS database credentials are different, connect to the server using the | ||
| // right ones, to ensure we have access to the $database and can see it in information_schema | ||
| if ($database != $jConfig->db) { | ||
| CRM_Core_DAO::init(CIVICRM_DSN); |
Contributor
Author
There was a problem hiding this comment.
Note 1:
This seems to be necessary, and works, even though we (apparently) don't set up the autoloading until later in civicrm_setup_instance().
Possibly we should factor the autoloading out and put it earlier in the process, to make sure that CRM_Core_DAO exists? Or get rid of the autoloading entirely, if (as it seems) it has already been done at a higher level?
Note 2:
We could simplify this further and just use CRM_Core_DAO always, getting rid of Joomla database calls entirely?
webmaster-cses-org-uk
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a proposed fix to issue #60, in which it was found that the query to detect the presence (or otherwise) of the
civicrm_domaintable incivicrm_detect_upgrade()fails if the CiviCRM and CMS databases are different and have different credentials / user access.Before
Upgrades are treated as new installations if the database user for the CMS (Joomla!) database has no privileges to access the CiviCRM database (given that the databases are different).
This causes several issues, such as:
CIVICRM_DSNand other key settings are not copied across in thecivicrm.settings.phpfiles, i.e. #47 is not fixed.civicrm_XXXtables keep getting created in the CMS database.And possibly others I've not noticed.
After
Upgrades are correctly detected, by using the CiviCRM database client to access the
information_schema, instead of the CMS database client, as appropriate.All of the above problems go away.
Notes
This PR also changes the logic so that existing settings are copied from
civicrm.settings.phpas long as that file is readable, irrespective of whether it is an upgrade or not. This makes the behaviour ofcivicrm_setup_instance()consistent withcivicrm_detect_upgrade(), and feels more appropriate. The rationale being, if someone has edited the settings file, you'd like to preserve this in any case.This could be simplified further, to always use the CiviCRM database client. After all, we are only interested in checking whether a CiviCRM table exists.