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

make the SQLite driver work #15

Merged
merged 6 commits into from Feb 14, 2012
Merged

make the SQLite driver work #15

merged 6 commits into from Feb 14, 2012

Conversation

rjbs
Copy link
Contributor

@rjbs rjbs commented Feb 14, 2012

For detailed notes on the critical bugfixes in this pull request, see my commit comments.

this test is very very close to the code in the svp docs
the former exists; the latter does not
For example, say I do this:

  #!perl
  use DBIx::Connector;
  my $conn = DBIx::Connector->new('dbi:SQLite:db.lite', undef, undef);
  $conn->txn({ ... $conn->svn({ ... }); });

Creating the connector loads DBIx::Connector::Driver::SQLite, which,
*immediately* and during *compile time* checks
$DBD::SQLite::sqlite_version to decide whether or not to create the
methods that make the thing useful at all.

Unfortunately, DBD::SQLite isn't loaded until the first call to txn, so
checking that variable isn't very useful yet.  The
"$...::sqlite_version || 0" is a clue: why would that *ever* be
undefined?

I've replaced all that logic with runtime checks.  After all, you
certainly won't call txn or svp until *after* you've connected, which
means *after* DBD::SQLite has been loaded.  I've also made it fatal to
try to use these features without the right version of SQLite.  I'd hate
to think someone is happily carrying on using savepoints and thinking
they're useful, only to have each invocation silently do nothing!
@theory
Copy link
Collaborator

theory commented Feb 14, 2012

Comment on the SQLite cleverness fix: Since I wrote that code (it should have used DBD::SQLite, frankly), I added the _connect method. So I think it would make sense to stick the check there, so that you don't have to call it in every other method.

As for the SQLite test: could it be adapted into t/svp_live.t? So that it can then run against any driver?

The fix is obviously correct, though. Thanks!

@rjbs
Copy link
Contributor Author

rjbs commented Feb 14, 2012

I'll have a look at moving the test and check tomorrow morning my time, so hopefully it'll be waiting for you when you get up. :-)

@theory
Copy link
Collaborator

theory commented Feb 14, 2012

Thanks!

@rjbs
Copy link
Contributor Author

rjbs commented Feb 14, 2012

There you go!

theory added a commit that referenced this pull request Feb 14, 2012
Make the SQLite driver work and fix bad method call in Driver.pm.
@theory theory merged commit e39f76b into ap:master Feb 14, 2012
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.

None yet

2 participants