Fixes #1349 - Gives option to successfully setup multiple DB connections #5255

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Collaborator

Srokap commented Mar 20, 2013

Just rebased #410 (see it's comments)

Collaborator

Srokap commented Mar 20, 2013

@cash Any clue why I'm failing with such error? https://travis-ci.org/Elgg/Elgg/jobs/5648203 Alignment seems to be fine, or I'm missing something.

Owner

ewinslow commented Mar 20, 2013

The var names align, but you need to also align the start of the comments on those vars.

Example:

string   $var        Comment
stdClass $anotherVar More comments
Owner

ewinslow commented Apr 29, 2013

Needs rebase and style fixes

ewinslow closed this Apr 29, 2013

Contributor

cash commented Apr 29, 2013

@ewinslow @mrclay Before rebasing, we should answer the question of whether this is what we want. Is the only reason this wasn't pulled in was because of the coding standard issues?

@mrclay mrclay commented on the diff Apr 29, 2013

engine/settings.example.php
@@ -72,6 +72,29 @@
$CONFIG->dbprefix = '{{dbprefix}}';
+/*
@mrclay

mrclay Apr 29, 2013

Owner

Users shouldn't need to load/instantiate classes to configure this. Better to have the class be used by core in the boot process.

@cash

cash Apr 29, 2013

Contributor

@mrclay, are you suggesting something like this:

$CONFIG->db['read'][0] = array(
    'dbuser' => 'fred',
    'dbpassword' => 'xxxxx',
    ...
);

$CONFIG->db['write'] = array(
    'dbuser' => 'fred',
    'dbpassword' => 'xxxxx',
    ...
);

and then when we init the database we pass $CONFIG into the database config class constructor.

@mrclay

mrclay Apr 29, 2013

Owner

Yes. Elgg would pick a random read credentials set from $CONFIG->db['read']. If a user needs a non-random selection process, she can implement that logic and just give us one credential set in $CONFIG.

@mrclay mrclay commented on the diff Apr 29, 2013

engine/classes/ElggDatabaseConfig.php
@@ -0,0 +1,131 @@
+<?php
+/**
+ * Class used as part of multiple database connections configuration.
+ * It defines sets of connection parameters for different types of connection links.
+ *
+ * @package Elgg.Core
+ * @subpackage Database
+ */
+class ElggDatabaseConfig implements ArrayAccess {
@mrclay

mrclay Apr 29, 2013

Owner

Is ArrayAccess really needed? To me this nearly always adds bloat and doesn't add value to the API.

@mrclay mrclay commented on the diff Apr 29, 2013

engine/classes/ElggDatabaseConfig.php
+ global $CONFIG;
+ foreach ($conf as $k => $v) {
+ $CONFIG->$k = $v;
+ }
+ }
+
+ /**
+ * Get config by the policy. Random by default.
+ *
+ * @param string $dblinkname Name of the type of DB link, usually read|write
+ * @return stdClass|null returns random config of specified class or returns null on failure.
+ */
+ public function getConfig($dblinkname) {
+ if (isset($this->data[$dblinkname])) {
+ if (is_array($this->data[$dblinkname])) {//get random
+ $index = rand(0, count($this->data[$dblinkname]) - 1);
@mrclay

mrclay Apr 29, 2013

Owner

array_rand() is clearer, not a big deal though

Owner

ewinslow commented Apr 29, 2013

Just cleaning up old PRs. I assume we do want this feature to work, though...

Contributor

cash commented May 4, 2013

db config implementation here: #5434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment