- Download Diff

Summary:

[Gravity] Made a SQLite3QueryResult class and some classes to support it

Review Request #414 — Created Feb. 9, 2016 and submitted May 26, 2016, 5:57 p.m.

Information
Owner:	rubendgr
Repository:	Lunr
Branch:	rubendgr:sqlite_result
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Implementation of the SQLite3QueryResult class and tests to support it.

Testing Done

Unit test.

Issues

0
0
23
0
All Issues: 23

Description	From	Last Updated
no need for this if not used	tardypad	Feb. 12, 2016, 1:20 p.m.
to be removed as well	tardypad	Feb. 12, 2016, 1:20 p.m.
This could be even shorter by just returning the boolean result. return ($this->error_number == self::DEADLOCK_ERR_CODE);	smillernl	Feb. 12, 2016, 1:17 p.m.
I guess you still need to return something that "makes sense". Any code can call this function and should know ...	tardypad	Feb. 12, 2016, 1:20 p.m.
The newer php array syntax is prefered. [] over array()	smillernl	Feb. 12, 2016, 1:16 p.m.
This is not needed if you call the set up method setUp() in SQLite3QueryResultTest	smillernl	Feb. 12, 2016, 1:16 p.m.
The LunrBaseTest class has a function for this: get_accessible_reflection_property($property) And in this case you could even use the assertPropertySame($property, $expected) ...	smillernl	Feb. 12, 2016, 1:15 p.m.
LunrBaseTests set_reflection_property_value is an easy replacement for this.	smillernl	Feb. 12, 2016, 1:14 p.m.
assertEquals tests only the value of the parameters while assertSame tests the type as well. Saves you some lines.	smillernl	Feb. 12, 2016, 1:13 p.m.
this needs to be setUp() or it wont be run :(	smillernl	Feb. 12, 2016, 1:12 p.m.
This is not how mocks are supposed to be created. You either use the mock builder for PHPUnit to automatically ...	tardypad	Feb. 12, 2016, 1:18 p.m.
this can't be removed like that some projects most likely already use it	tardypad	Feb. 17, 2016, 10:16 a.m.
TRUE	tardypad	Feb. 17, 2016, 10:04 a.m.
If this was missing before, don't make it part of this review but create another one This is confusing if ...	tardypad	Feb. 17, 2016, 10:02 a.m.
This does not mean what you think it means. A query might have rows, but it's just string formatting depending ...	pprkut	Feb. 18, 2016, 10:18 a.m.
This is not in tests though (no big deal but i'm nitpicking anyways)	smillernl	March 25, 2016, 1:10 p.m.
No need for ? TRUE : FALSE ($this->error_number == self::DEADLOCK_ERR_CODE) will already return a boolean.	smillernl	March 25, 2016, 1:11 p.m.
what about SQLiteResult->numRows()?	pprkut	March 25, 2016, 1:05 p.m.
pls use [] instead of array()	pprkut	Feb. 18, 2016, 10:16 a.m.
[]	smillernl	March 25, 2016, 1:12 p.m.
[]	smillernl	March 25, 2016, 1:12 p.m.
We don't generally define the Tests part of the namespace in the package. (again no big deal)	smillernl	March 25, 2016, 1:14 p.m.
Many of the functions here are missing an @covers annotation.	smillernl	March 25, 2016, 1:36 p.m.

It feels like many unit tests are missing if you compare with the way the MySQLResult class is tested

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 1)
Show all issues
```
no need for this if not used
```
1. rubendgr Feb. 12, 2016, 1:27 p.m.
  Removed.
src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 1)
Show all issues
```
to be removed as well
```
1. rubendgr Feb. 12, 2016, 1:27 p.m.
  Removed.

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 1)

Show all issues

I guess you still need to return something that "makes sense".

Any code can call this function and should know what to do with the result. Here there is nothing.

Maybe the comment of the interface function should be updated as well to explain the behavior

rubendgr Feb. 12, 2016, 1:27 p.m.

I deleted the number_of_rows from the DatabaseQueryResultInterface. Now SQLite3QueryResult is not forced to use this method anymore.

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php (Diff revision 1)

Show all issues

This is not how mocks are supposed to be created.

You either use the mock builder for PHPUnit to automatically create a mock of the class you want.

Or you define manually a mock class yourself if really needed (this happens rarely)

Here you seem to mix things

rubendgr Feb. 12, 2016, 1:27 p.m.
```
Not making a mock of a mock anymore.
```

Branch:

-	rubendgr:review
+	rubendgr:sqlite_result

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 1)

Show all issues

This could be even shorter by just returning the boolean result.

return ($this->error_number == self::DEADLOCK_ERR_CODE);

rubendgr March 16, 2016, 12:01 p.m.

I kept it for being consistent with other code.

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 1)
Show all issues
```
The newer php array syntax is prefered.
[] over array()
```
1. rubendgr March 16, 2016, 12:01 p.m.
  Fixed.

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php (Diff revision 1)

Show all issues

This is not needed if you call the set up method setUp() in SQLite3QueryResultTest

rubendgr March 16, 2016, 12:01 p.m.

I kept it because I have three versions of the setUp.

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php (Diff revision 1)

Show all issues

The LunrBaseTest class has a function for this:
get_accessible_reflection_property($property)
And in this case you could even use the assertPropertySame($property, $expected) function.

rubendgr March 16, 2016, 12:01 p.m.
```
Using this method now.
```

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php (Diff revision 1)
Show all issues
```
LunrBaseTests set_reflection_property_value is an easy replacement for this.
```
1. rubendgr March 16, 2016, 12:01 p.m.
  Using this method now.

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php (Diff revision 1)

Show all issues

assertEquals tests only the value of the parameters while assertSame tests the type as well. Saves you some lines.

rubendgr March 16, 2016, 12:01 p.m.

Changed assertInternalType and assertEquals to assertSame. With arrays this was not working.

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php (Diff revision 1)
Show all issues
```
this needs to be setUp() or it wont be run :(
```
1. rubendgr March 16, 2016, 12:01 p.m.
  Changed to setUp when needed.

Change Summary:

Added four new test classes for SQLite3QueryResult and improved the existing classes for SQLite3QueryResult

Diff:

Revision 2 (+781 -132)

Show changes

	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/MockSQLite3Result.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php
	1 more

Change Summary:

Changed the DatabaseQueryResultInterface so SQLite3QueryResult is not forced to use number_of_rows() which is not supported.

Diff:

Revision 3 (-17)

Show changes

	src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php

Description:

~		Reviewed at http://reviews.lunr.nl/r/413/
	~	Implementation of the SQLite3QueryResult class and tests to support it.

Change Summary:

Corrected the diff.

Diff:

Revision 4 (+1189 -8)

Show changes

	src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php
	1 more

Change Summary:

Applied the M2mobi coding standards to the classes.

Diff:

Revision 5 (+1234 -8)

Show changes

	src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php
	1 more

```
The rest looks fine
```

src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php (Diff revision 5)

Show all issues

this can't be removed like that
some projects most likely already use it

rubendgr March 16, 2016, 12:01 p.m.

Put number_of_rows back in the interface. Now the method is returning -1 instead of the number of rows.

src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php (Diff revision 5)
Show all issues
```
TRUE
```
1. rubendgr March 16, 2016, 12:01 p.m.
  Fixed.

tests/phpunit.xml (Diff revision 5)

Show all issues

If this was missing before, don't make it part of this review but create another one
This is confusing if you mix different unrelated things in a same review

rubendgr March 16, 2016, 12:01 p.m.
```
Yes, this was a fix for MySQL.
```

Diff:

Revision 6 (+1248 -4)

Show changes

	src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php
	1 more

Ship it!

```
Ship It!
```

src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php (Diff revision 6)

Show all issues

This does not mean what you think it means. A query might have rows, but it's just string formatting depending on where in the query I set a line-break, and even that only when I type it manually ;)
The query we generate only has one row.

rubendgr Feb. 18, 2016, 1:53 p.m.

I changed it to the number of rows in the result set.

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)

Show all issues

what about SQLiteResult->numRows()?

pprkut Feb. 18, 2016, 10:13 a.m.

Ok, so that is for sqlite2, not 3. But still, -1 is not really an appropriate reply. We know the row count, we just have to work a bit harder to get it.

rubendgr Feb. 18, 2016, 1:53 p.m.

/

     * Returns the number of rows in the result set.



     * @return Integer $number Number of rows in the result set.

/

    public function number_of_rows()

    {

        $count = 0;
    while ($this->result->fetchArray(SQLITE3_ASSOC))
    {
        $count++;
    }

    $this->free_result();

    return $count;
}

Is this a good solution ?

pprkut Feb. 22, 2016, 2:38 p.m.

almost. You want to somehow interact with the result() functions, since otherwise you either can only check row count or fetch results, or fetch results twice. Neither is what we want.

rubendgr March 16, 2016, 1:19 p.m.

/

     * Returns the number of rows in the query.



     * @return Integer $number Number of rows in the result set.

/

    public function number_of_rows()

    {

        $count = 0;
    while ($this->result->fetchArray(SQLITE3_ASSOC))
    {
        $count++;
    }

    return $count;
}

I removed the free_result() function. Is the code ready to ship ?

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)
Show all issues
```
pls use [] instead of array()
```
1. rubendgr Feb. 18, 2016, 1:53 p.m.
  Fixed.

Make sure you check the other files for the comment changes as well. Didn't mark all of them.

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)
Show all issues
```
This is not in tests though (no big deal but i'm nitpicking anyways)
```
src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)
```
Usually has email address (yay, nitpicking)
```

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)

Show all issues

No need for ? TRUE : FALSE
($this->error_number == self::DEADLOCK_ERR_CODE) will already return a boolean.

src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)
Show all issues
```
[]
```
src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php (Diff revision 6)
Show all issues
```
[]
```
src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php (Diff revision 6)
Show all issues
```
We don't generally define the Tests part of the namespace in the package. (again no big deal)
```
src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php (Diff revision 6)
Show all issues
```
Many of the functions here are missing an @covers annotation.
```
src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php (Diff revision 6)
```
See previous comments
```
src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php (Diff revision 6)
```
See previous comments
```

Change Summary:

Changed the number_of_rows function and did some small fixes to the comments.

Diff:

Revision 7 (+1288 -4)

Show changes

	src/Lunr/Gravity/Database/DatabaseQueryResultInterface.php
	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php
	1 more

Change Summary:

Removed some cover annotations for properties.

Diff:

Revision 8 (+53 -31)

Show changes

	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultTest.php

There are no tests for the number_of_rows() method.

Change Summary:

Added a test for number of rows.

Diff:

Revision 9 (+52 -29)

Show changes

	src/Lunr/Gravity/Database/SQLite3/SQLite3QueryResult.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultBaseTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFailedTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultFreeTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultNoResultTest.php
	src/Lunr/Gravity/Database/SQLite3/Tests/SQLite3QueryResultResultTest.php

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Gravity] Made a SQLite3QueryResult class and some classes to support it

Branch:

Change Summary:

Diff:

Change Summary:

Diff:

Description:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: