Compound primary keys support (revisited!)

[lrlopez]

This PR adds support for compound primary keys. It is fully backward compatible with one column primary keys tables and also works with Paris (although I recommend a deeper compatibility test).

As a side-effect, I've enhanced where_*() and having_*() methods to allow stating multiple column-names/values at once (is needed for the compound keys support).

All the changes are documented and I've also thrown in some tests (beware: I only test that SQL queries are built the right way).

I've splitted the PR into several commits, but I could squash them into one big commit if this gets merged in order to not pollute the history.

Closes #161

[charsleysa]

@lrlopez I just tried implementing this and found 3 flaws with Idiorm and your patch.

1. $this->id() returns an associative array, I'm assuming this is intended functionality.
   If so, then it is being used wrong. If not then there's your first problem.
   Now when you have compound primary keys is the only time you'll notice this problem.
   Biggest issue with this is that it passes an associative array to $this->_execute() and you cant do that.

2. Next issue is that the logger that logs these queries doesn't care that you give it an associative array.
   So when it's generating the query in the logger it will generate fine as long as the parameters are in the right order.
   The logger should strip out any associative indexes and only keep integer based indexes.

3. The reason the tests didn't fail is because the test's don't bother to check for this.
   The MockPDOStatement objects should not have an empty ->execute() function.
   The function should check that the parameters array is correctly formatted (e.g. not an associative array).

In conclusion, if you could please fix these issues that would be great.
I would do it myself but I'm already behind on my project.
I look forward to hearing from you.

Cheers

[lrlopez]

Stefan, thank you very much for reviewing the PR.

You're right. At first sight, it seems to be an easy fix though, as we'd just need to enclose $this->id() into an array_value() call.

I'll do some testing and amend the commit once I arrive home.

[lrlopez]

I've reviewed again the code and found just one direct use of $this->id() when calling _execute(). I've fixed it.

The PDO mock object is too weak to make some automated tests of this feature, so I hope to get some additional time this weekend to do a throughful test using real code.

[treffynnon]

I don't think I am missing anything, but $model->id() is the preferred
public way of getting access to a models id. It is in user land code all
over so its signature can't change if that is what you're proposing.

[lrlopez]

Simon, I'm not 100% sure about what are you referring to, but I think the method signature hasn't changed. It just returns an associative array when compound primary keys are used; in any other case just a string.

[treffynnon]

Cool. I wasn't referring to the code when I wrote my last email just the
comments you two were making so I couldn't be sure and wanted to check.

On Jan 11, 2014 7:42 PM, "Luis Ramón López" notifications@github.com
wrote:

Simon, I'm not 100% sure about what are you referring to, but I think the
method signature hasn't changed. It just returns an associative array when
compound primary keys are used; in any other case just a string.

—
Reply to this email directly or view it on GitHub.

[charsleysa]

@lrlopez @treffynnon I'm going to update the MockPDO objects and the logger to check for correct integer index count on arrays as that is what a real PDO object does.

This should help diagnose any problems related to the compound keys and help the testing correctly represent the real environments.

[charsleysa]

See #173

[treffynnon]

I have merged this PR into develop, thank you. Please help to test it.

@lrlopez @charsleysa @tag

[lrlopez]

Thanks @treffynnon! I'll do and report back if I find something wrong.