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

Multiple connections implementation #76

Closed
wants to merge 7 commits into from
Closed

Conversation

tag
Copy link
Contributor

@tag tag commented Nov 24, 2012

Addresses #15. Also includes pull request #75 (a consequence of me still getting comfortable with git and github).

Pull request is to your dev branch, so you can tinker/adjust/etc., as needed. Change log not updated in this commit.

Implements multiple connections, including documentation and unit tests.
Utilizes key names to distinguish connections, but uses a default
connection if none specified. I don't (yet) use multiple connections in
my work (it's pending), so this has not been tested "in the wild".

Added unit tests with additional connections, ran unit tests for Paris
against this build too, so an unmodified Paris is forward-compatible
with this commit (mulitple connections support not yet coded for Paris).

Does NOT add support for queries across multiple connections. (I
don't even want to go there ...)

Edge-case compatibility breaks:
  • ORM::_setup_identifier_quote_character visibility was changed to
    protected from public (which was likely original intent, judging by prefixed name)
  • May break compatibility if ORM has been extended, and subclasses
    directly utilize ::_config, ::_db, ::_query_log, or
    ::_query_cache instead of using pre-existing accessor methods. (Paris
    does not do this; all Paris tests pass)
  • Re-use of Tester class outside of Idiorm repo, as
    Tester::check_equal() was renamed to Tester::check_equal_query()
Other notes

New method: ORM::get_connection_keys().
New Tester method: Tester::check_equal_string().

TODO: Consider adding methods to get (connection-specific)
configuration info.

tag added 4 commits November 21, 2012 17:24
Mutiple connections code, including documentation and unit tests.
Utilizes key names to distinguish connections, but uses a default
connection if none specified. I don't (yet) use multiple connections in
my work (it's pending), so this has not been tested "in the wild".

Added unit tests with additional connections, ran unit tests for Paris
against this build too, so an unmodified Paris is forward-compatible
with this commit (mulitple connections support not yet coded for Paris).

Does *NOT* add support for queries across multiple connections. (I
don't even want to go there ...)

##### Edge-case compatibility breaks:

* ORM::_setup_identifier_quote_character visibility was changed to
protected (which was likely original intent, judging by prefixed name)

* May break compatibility if ORM has been extended, **and** subclasses
directly utilize `::_config`, `::_db`, `::_query_log`, or
`::_query_cache` instead of using pre-existing accessor methods. (Paris
does not do this; all Paris tests pass)

* Re-use of `Tester` class outside of Idiorm repo, as
`Tester::check_equal()` was renamed to `Tester::check_equal_query()`

##### Other notes
New method: `ORM::get_connection_keys()`.
New `Tester` method: `Tester::check_equal_string()`.

TODO: Consider adding methods to get (connection-specific)
configuration info.
tag pushed a commit to tag/paris that referenced this pull request Dec 19, 2012
Finishes Paris side of j4mie/idiorm#15. Requires (of course) pull request at j4mie/idiorm#76, which is the Idiorm implementation.

Unit tests and documentation included. Unit tests work, but I haven't yet tried this on "real" data.
@ghost ghost assigned treffynnon Dec 19, 2012
@treffynnon
Copy link
Collaborator

I just wanted to re-iterate my comments here to keep it all together:

Thank you for your hard work! This is looking near completion.

A couple of things:

  • I am not keen on the variable naming of $which - wouldn't this be clearer as $connection?>
  • Same goes for $_which_db
  • I prefer Mock to Dummy when naming mock objects

I haven't had a chance to run the patch yet, but otherwise it is looking good and ready for a pull request.

And also make another suggestion for this pull request. Instead of changing the API of configure I think it would be nice to specify the connection using dot notation.

So to configure the default instance it would be:

ORM::configure('something', true);

But if you wanted to specify a certain connection it would be:

ORM::configure('db2.something', true);

This way we are not changing the external API also it is similar to the SQL syntax for specifying a database.

It would be nice if this could also be applied to for_table, etc. so it could be something like:

ORM::for_table('db2.my_table_name')->find_many();

What do you think @tag ? Also do you foresee any issues with that? I had just thought it would be a nice succinct and easy way to keep the API stable.

@treffynnon
Copy link
Collaborator

I also forgot to mention here that I am hoping to get the 1.3.0 releases of Idiorm and Paris out for the end of the month. Does this fit with your timelines?

@treffynnon
Copy link
Collaborator

Just another thing to note. Is that I am slowly moving all tests over into PHPUnit to standardise the test suite. This is just so that method tests and query generation tests all appear and are coded in the same way. The eventual aim is to get Travis-CI running the tests on each commit.

Please see https://github.com/j4mie/idiorm/tree/develop/test for the classes.

Tom Gregory added 2 commits January 21, 2013 17:28
…ed documentation to rat format, and tests to use phpunit.

(Resolved)Conflicts:
	README.markdown
	idiorm.php
	test/test_queries.php
@tag
Copy link
Contributor Author

tag commented Jan 22, 2013

Okay ... the current state of the develop branch has been merged into my multiple connection branch (which was significantly more time consuming than I'd planned, given the many changes in develop over the past two months).

I've reimplemented the multiple connection tests using phpunit. Some are in their own class, but in a couple of cases, it made since to integrate them in to existing test classes (where they should all be, I think, as running a test on multiple connections duplicates code on a single connection).

As a consequence of redoing these tests, I discovered an (unintended) consequence of the multiple connections implementation: if the connection name is mis-typed, a new in-memory sqlite database is automatically created under the bad connection name. This is a consequence of Idiorm supplying a default connection_string in its configuration.

A note on the dot-product approach: I've been going back and forth on this. Part of me thinks it's a great idea (and should be simple enough to implement in addition to the way I already implemented it. However, such an approach would only work with setting configuration options via ORM::configure(). The additional parameter approach would still be needed for every other publicly exposed method, including ORM::raw_execute() and ORM::for_table(), to list only two examples. The advantage of maintaining the optional parameter approach is greater cognitive simplicity. If you want the alternative of using the dot notation for configure, though, it should be easy enough to grab the left-most string separated by a dot.

@tag
Copy link
Contributor Author

tag commented Jan 22, 2013

I see that you already mentioned for_table() as an example when using the dot notation for multiple connections. I'm also worried that might be a backward compatibility break, if some developers (using, e.g., mysql) specify their database name.

@treffynnon
Copy link
Collaborator

Ah I thought that might be the case with the dot notation. It was something I dreamed up on the train home.

Thanks and I will take a look your changes as soon as I can.

tag pushed a commit to tag/paris that referenced this pull request Jan 22, 2013
Finishes Paris side of j4mie/idiorm#15. Requires (of course) pull request at j4mie/idiorm#76, which is the Idiorm implementation.

Unit tests and documentation included. Unit tests work, but I haven't yet tried this on "real" data.
tag pushed a commit to tag/paris that referenced this pull request Jan 22, 2013
…rted multiple connection tests to phpunit and multiple connection documentation to rst.

Completes multiple connection implementation of Paris, as discussed in j4mie/idiorm#76.

(Resolved) Conflicts:
	README.markdown
	test/idiorm.php
	test/test_classes.php
	test/test_queries.php
tag pushed a commit to tag/paris that referenced this pull request Jan 22, 2013
…rted multiple connection tests to phpunit and multiple connection documentation to rst.

Completes multiple connection implementation of Paris, as discussed in j4mie/idiorm#76.

(Resolved) Conflicts:
	README.markdown
	test/idiorm.php
	test/test_classes.php
	test/test_queries.php
@tag
Copy link
Contributor Author

tag commented Jan 22, 2013

Multiple connections for Paris now complete and in my 'dev-multi' branch. Tests (phpunit) and docs (rst) included.

(I'm not sure what happened with the second commit to tag/paris@d0d3d8b, but that has been un-done. My dev-multi branch is now correct.)

@treffynnon
Copy link
Collaborator

@treffynnon treffynnon closed this Jan 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants