- Download Diff

Summary:

[Gravity] add SQLiteConnection class

Review Request #83 — Created April 16, 2013 and discarded June 14, 2013, 2:54 p.m.

Information
Owner:	dinos
Repository:	Lunr
Branch:	dinostheo:SQLiteConnection
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Here is the first step of the implementation of SQL lite.
the class for query result and query builder will follow.

This depends on http://reviews.lunr.nl/r/94/

Testing Done

unit tests

Issues

7
0
11
0
All Issues: 18

Description	From	Last Updated
wrong	pprkut	April 18, 2013, 4:54 p.m.
wrong namespace	pprkut	April 18, 2013, 4:54 p.m.
keep this compatible with the mysql connection	pprkut	April 18, 2013, 4:55 p.m.
that looks like a very weird key to use	pprkut	April 18, 2013, 4:55 p.m.
doesn't exist yet	pprkut	April 18, 2013, 4:56 p.m.
you need to make sure this actually makes sense for sqlite	pprkut	April 18, 2013, 4:57 p.m.
doesn't exist yet	pprkut	April 18, 2013, 4:58 p.m.
doesn't exist yet	pprkut	April 18, 2013, 4:58 p.m.
nitpick. You are trying to do the right thing, but you are drawing the wrong conclusion. In order to avoid ...	pprkut	May 21, 2013, 3:52 p.m.
Doesn't exist yet	pprkut	May 21, 2013, 3:52 p.m.
you can't instantiate an interface....	pprkut	May 21, 2013, 3:52 p.m.
We will need a SQLite3 wrapper class in order to avoid the connect on construct issue.	pprkut	May 30, 2013, 12:38 p.m.
How do you unit test this?	pprkut	May 30, 2013, 12:38 p.m.
superfluous	pprkut	May 30, 2013, 12:38 p.m.
There's just way too many uncertainties about this test. How do you ensure that connect() will always work?	pprkut	May 30, 2013, 12:38 p.m.
escape_string, when not connected, will try to connect. Yet, this is not reflected in this test.	pprkut	May 30, 2013, 12:38 p.m.
Why not Mock the SQlite3 class?	pprkut	May 30, 2013, 12:38 p.m.
?here does $this->logger come from?	pprkut	May 30, 2013, 12:38 p.m.

Please rebase on top of http://reviews.lunr.nl/r/77/

Change Summary:

Rebased against pprkut/gravity
SQLiteConnection class moved to Gravity/Database/SQLite namespace

Diff:

Revision 2 (+264)

Show changes

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php

Change Summary:

Little correction in file and class header

Diff:

Revision 3 (+264)

Show changes

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
wrong
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
wrong namespace
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
keep this compatible with the mysql connection
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
that looks like a very weird key to use
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
doesn't exist yet
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
you need to make sure this actually makes sense for sqlite
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
doesn't exist yet
```
src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 3)
Show all issues
```
doesn't exist yet
```

There's a lot of issues solved but no new revision. Please only close issues when posting a new revision.

Change Summary:

Update according to dinos review for delimiter.

Description:

		Here is the first step of the implementation of SQL lite.
		the class for query result and query builder will follow.
	+
	+	This depends on http://reviews.lunr.nl/r/93/

Diff:

Revision 4 (+269)

Show changes

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 4)

Show all issues

nitpick. You are trying to do the right thing, but you are drawing the wrong conclusion.
In order to avoid the line length limit it would make much more sense to reduce the number of function calls than just introduce a (pointless) placeholder variable

olivier May 21, 2013, 3:52 p.m.

This method will be deleted as we already give the file to the SQLite3 object.

pprkut May 21, 2013, 4:32 p.m.

Our db abstraction layer is not set out to work well with this scenario. The whole design of the architecture is based on "connect on demand". Now, it seems that's not possible with SQLite3. That leaves us with two options:

* Pretend the "connect on __construct()" never happened.
* Don't do connect on demand, which might have various repercussions on MySQL.

Your new revision however isn't doing either of those...

pprkut May 21, 2013, 4:40 p.m.

Woops, forgot, there is a 3rd option:

* Create a simple SQLite3 Composite wrapper that allows construct() without connect.

olivier May 21, 2013, 6:24 p.m.

Anyway deleting it was no good option.
I prefer to pretend we are already connected then instead of not.

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 4)
Show all issues
```
Doesn't exist yet
```
1. olivier May 21, 2013, 3:52 p.m.
  Yeld NULL then before having the class
2. pprkut May 21, 2013, 4:32 p.m.
  That's fine

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 4)

Show all issues

you can't instantiate an interface....

olivier May 21, 2013, 3:52 p.m.

Yeld NULL then before having the class
same here

pprkut May 21, 2013, 4:32 p.m.

That's conceptually fine, but you still have to do it in a way you can implement reasonable unit tests. That's not the case with the current code.

olivier May 21, 2013, 6:24 p.m.

Unit testing as I cannot mock the SQLite3 object...

pprkut May 21, 2013, 6:45 p.m.
```
why can't you mock the SQLite3 object?
```

Change Summary:

Remove use of configuration:

The file is already given to the SQlite3 object
The driver can cause issue if concurrent use with other DB

Summary:

-	[DataAccess][RFC] add SQLiteConnection class
+	[Gravity][RFC] add SQLiteConnection class

Diff:

Revision 5 (+249)

Show changes

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php

Change Summary:

Update dependency

Description:

		Here is the first step of the implementation of SQL lite.
		the class for query result and query builder will follow.

~		This depends on http://reviews.lunr.nl/r/93/
	~	This depends on http://reviews.lunr.nl/r/94/

Change Summary:

change made according to review.
the connection is considered connected from the start

Diff:

Revision 6 (+281 -4)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php

Change Summary:

Change made acccording to reveiw
the coneection is considered open from the start

Diff:

Revision 7 (+263)

Show changes

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php

Change Summary:

Added unit tests to the SQLiteConnection class of Olivier. 
2 functions from this class are not tested due to their dependency to query builder, get_new_dml_query_builder_object & query.

Diff:

Revision 8 (+905 -3)

Show changes

	src/Lunr/Gravity/Database/DatabaseDMLQueryBuilder.php
	src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php
	src/Lunr/Gravity/Database/SQLite/Tests/MockSQLite3.php
	src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionBaseTest.php
	src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionConnectTest.php
	src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionEscapeTest.php
	src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionSetTest.php
	src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionTest.php
	1 more

Testing Done:

unit tests

Summary:

-	[Gravity][RFC] add SQLiteConnection class
+	[Gravity] add SQLiteConnection class

Branch:

-	https://github.com/owizen/lunr/tree/SQLiteConnection
+	dinostheo:SQLiteConnection

Change Summary:

Delete reference to non-existing bug

Bugs:

-	14955

Fix it!

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 8)

Show all issues

We will need a SQLite3 wrapper class in order to avoid the connect on construct issue.

dinos May 30, 2013, 2:40 p.m.
```
I didn't understand this comment.
```

pprkut May 30, 2013, 2:48 p.m.

It's about what I discussed with Olivier further above.

pprkut June 10, 2013, 4:45 p.m.

Now that we have a SQLite wrapper class can you adjust the documentation and constructor?

src/Lunr/Gravity/Database/SQLite/SQLiteConnection.php (Diff revision 8)

Show all issues

How do you unit test this?

dinos May 30, 2013, 2:40 p.m.

I am not. 
This code can change with a TODO as it is to be written with the SQLiteDMLQueryBuilder

pprkut May 30, 2013, 2:48 p.m.

You can still do reasonable unit tests with the half-implemented code. As long as it's actually testable. In this case you can't distinguish between connected and not connected.

src/Lunr/Gravity/Database/SQLite/Tests/MockSQLite3.php (Diff revision 8)

Show all issues

superfluous

dinos May 30, 2013, 2:40 p.m.

I thought we needed a Mock class as mocking SQLite3 was not working.

src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionConnectTest.php (Diff revision 8)

Show all issues

There's just way too many uncertainties about this test. How do you ensure that connect() will always work?

dinos May 30, 2013, 2:40 p.m.

The test is wrong. I know see what's going on. I will fix this

src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionEscapeTest.php (Diff revision 8)

Show all issues

escape_string, when not connected, will try to connect. Yet, this is not reflected in this test.

pprkut June 10, 2013, 4:45 p.m.

Follow up, with the new sqlite3 wrapper class, escape_string no longer needs an open connection to escape a string. So this should be fine, as long as escape_string itself doesn't try to connect.

src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionTest.php (Diff revision 8)
Show all issues
```
Why not Mock the SQlite3 class?
```
1. dinos May 30, 2013, 2:40 p.m.
  Mocking SQLite3 wasn't working.
2. pprkut May 30, 2013, 2:48 p.m.
  define "not working"
src/Lunr/Gravity/Database/SQLite/Tests/SQLiteConnectionTest.php (Diff revision 8)
Show all issues
```
?here does $this->logger come from?
```

You have a pending review.

Review Board 3.0.24

[Gravity] add SQLiteConnection class

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Description:

Diff:

Change Summary:

Summary:

Diff:

Change Summary:

Description:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Testing Done:

Summary:

Branch:

Change Summary:

Bugs:

Status: Discarded