Issue 145 [Follow-up]: Use singleton for database connection

[veggiematts]

As pointed out in #245 , the patch recently merged in development as part of #292 only addressed the problem for the resource module.

This PR extends the use of a single DB connection to all modules.

[jeffnm]

@fondrenlibrary would you review this PR? If it can be merged, then we can close #145 and #245

[fondrenlibrary]

Got the following error when switching to this PR branch.

[veggiematts]

Do you have the same error when switching to the development branch?
Are there any relevant error messages in the the log files?

[fondrenlibrary]

Same error when switching to the development . No error spotted in the error log.

[t4k]

@fondrenlibrary I have experienced this when testing other PRs that change the common/configuration.ini file:

[installation_details]
version = "2.0.0"

Make sure the version isn't set to 2.1.0; that's when I've seen this problem.

[fondrenlibrary]

[Wed Jan 31 09:45:31.173891 2018] [:error] [pid 35085] [client 10.74.22.178:54606] PHP Fatal error: Uncaught exception 'Exception' with message 'There was a problem with the database: Unknown column 'R.acquisitionTypeID' in 'on clause'' in /var/www/html/CoralMaster/resources/admin/classes/common/DBService.php:52\nStack trace:\n#0 /var/www/html/CoralMaster/resources/admin/classes/common/DBService.php(91): DBService->checkForError()\n#1 /var/www/html/CoralMaster/resources/admin/classes/domain/Resource.php(1228): DBService->processQuery('SELECT R.resour...', 'assoc')\n#2 /var/www/html/CoralMaster/resources/ajax_htmldata/getSearchResources.php(35): Resource->search(Array, 'R.createDate DE...', '0, 25')\n#3 /var/www/html/CoralMaster/resources/ajax_htmldata.php(24): include('/var/www/html/C...')\n#4 {main}\n thrown in /var/www/html/CoralMaster/resources/admin/classes/common/DBService.php on line 52, referer: http://10.74.22.184/CoralMaster/resources/index.php

[veggiematts]

Sounds like your database and codebase are out of sync.

• If the code mentions R.acquisitionTypeID, then your database should be pre-multiple ordering:
  acquisitionTypeID is in the Resource table (currently: master)
• If the code mentions RA.acquisitionTypeID, then your database should be post-multiple ordering:
  acquisitionTypeID is in the ResourceAcquisition table (currently: development)

This PR is based on master, and your database seems to be on development.
Therefore, you should not switch to this PR to test it, but rather merging it to the development branch.

[fondrenlibrary]

Why this PR is based on the master branch instead of the development one? The standard way is not like this I think.

[veggiematts]

Another question could be: why would you switch branch instead of merging to the target branch? :)

That being said, seeing the previous commits on my branch, I was wrong to say that this PR is based on master, it's not, it's based on devel.
However, when this PR was made, multiple ordering was not already merged to devel, that's why it's not here.
That's one reason to always merge a PR on the target branch to test it (unless each PR is rebased everyday on the target branch, but no one does that, it's only done when conflicts arise)

The other reason to merge on the target branch instead of switching branch would be that you want to make sure that the development you're testing is correct towards the current state of the target branch, not the state of the branch at the time it started.

Does that make sense to you?

[t4k]

If it helps anyone, I sometimes use a workflow that involves patching instead of all the merging in order to test PRs.

I checkout the current development branch, and then I apply a patch for the pull request. A quick one-liner would be:
curl https://github.com/coral-erm/coral/pull/343.patch | git apply -v
or
wget -q -O - https://github.com/coral-erm/coral/pull/343.patch | git apply -v

This way it is always easy to undo changes for a single pull request.

@veggiematts Do you have a recommended testing procedure? Which pages or forms would be ideal to check against this PR to look for problems?

[fondrenlibrary]

After rebase the local biblibre-Issue_145_Followup_DB_Connections branch to the local development branch, the test can be done and I don't see any particular problems introduced by this PR. Thank @veggiematts and @t4k .

[jcuenod]

Don't know if this helps but that error message can be found in install/index.php.

In short, I believe that error means that index.php has not been correctly configured for the upgrade. So it's it's expecting $INSTALLATION_VERSION to match the last element in the array of versions

$INSTALLATION_VERSION = "2.1.0";
$INSTALLATION_VERSIONS = ["1.9.0", "2.0.0", "2.1.0"];

The rationale for this is that when this is changed it means that whoever the maintainer is updating this file is confident that the upgrade path works from 2.0.0 to 2.1.0.

[veggiematts]

Thanks @fondrenlibrary
@t4k I don't have any particular test plan for this, as it basically affects every database call made through an object. On a positive side, I guess that if something would be broken, everything would be broken.

[fondrenlibrary]

@t4k , to get a peace of mind, I also tested with our live license data, and can see this PR also solve the issue of displaying large number of child license documents which pr #245 is charged to solve.

[t4k]

@veggiematts @fondrenlibrary

Great! I'm confident in the testing from you two.

[t4k]

I'll be merging this tomorrow unless there are objections from anyone before then.