- Download Diff

Summary:

[Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Review Request #793 — Created Sept. 9, 2021 and submitted Sept. 27, 2021, 11:21 a.m.

Information
Owner:	b.stoop
Repository:	Lunr
Branch:
Bugs:
Depends On:
Blocks:	796
Commit:	96d04bd...
Reviewers
Groups:	lunr
People:

Description

Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Testing Done

Wrote script to test if it's working locally and made unit tests

Issues

0
0
16
0
All Issues: 16

Description	From	Last Updated
This looks like something you want to do once, not on every call to the method.	pprkut	Sept. 10, 2021, noon
this is never returned	pprkut	Sept. 10, 2021, noon
This won't work	b.stoop	Sept. 10, 2021, noon
mixed is for example mysqli_warning\|bool\|int mysqli_warning\|null is declared as ?mysqli_warning	pprkut	Sept. 13, 2021, 5:35 p.m.
Try to be specific in your checks. We read ! as !== TRUE and while that checks out here, it's ...	pprkut	Sept. 17, 2021, 9:58 a.m.
We like what we call "early exits". You can put a return statement inside the if. That way you can ...	pprkut	Sept. 17, 2021, 9:58 a.m.
use the helper method :) get_reflection_property_value()	pprkut	Sept. 20, 2021, 3:02 p.m.
We don't need that file :)	pprkut	Sept. 17, 2021, 10:06 a.m.
shouldn't be public, make it protected	pprkut	Sept. 22, 2021, 11:40 a.m.
Superfluous	pprkut	Sept. 22, 2021, 11:42 a.m.
Not correct ;)	pprkut	Sept. 22, 2021, 11:42 a.m.
Superfluous	pprkut	Sept. 22, 2021, 11:45 a.m.
Not correct ;)	pprkut	Sept. 22, 2021, 11:47 a.m.
2021 :)	pprkut	Sept. 24, 2021, 9:32 a.m.
2021 :)	pprkut	Sept. 24, 2021, 9:32 a.m.
2021 :)	pprkut	Sept. 24, 2021, 9:32 a.m.

Summary:

-	modified: src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
+	[Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Description:

~		[Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries
	~	Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Summary:

-	[Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries
+	[WIP] [Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Testing Done:

None

Change Summary:

Fixed code standard violations

Diff:

Revision 2 (+28)

Show changes

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 2)

Show all issues

This looks like something you want to do once, not on every call to the method.

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 2)
Show all issues
```
this is never returned
```

Change Summary:

Resolved issues

Diff:

Revision 3 (+31)

Show changes

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 3)

Show all issues

This won't work

Change Summary:

Fixed issues

Diff:

Revision 4 (+42)

Show changes

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 4)

Show all issues

mixed is for example mysqli_warning|bool|int
mysqli_warning|null is declared as ?mysqli_warning

Testing Done:

~		None
	~	Wrote script to test if it's working locally

Diff:

Revision 5 (+44)

Show changes

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php

Added Files:

Testing Done:

~		Wrote script to test if it's working locally
	~	Wrote script to test if it's working locally and made unit tests

Diff:

Revision 6 (+385)

Show changes

	test.php
	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	1 more

Removed Files:

Change Summary:

Improved unit test

Diff:

Revision 7 (+408)

Show changes

	test.php
	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	1 more

Looking good :)

Minor tweaks here and there

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 7)

Show all issues

Try to be specific in your checks. We read ! as !== TRUE and while that checks out here, it's not immediately obvious. By being explicit you make it clear that we check a boolean value here.

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 7)

Show all issues

We like what we call "early exits". You can put a return statement inside the if. That way you can drop the else clause and save one indentation level for the do-while loop.

src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php (Diff revision 7)

Show all issues

use the helper method :)
get_reflection_property_value()

b.stoop Sept. 17, 2021, 2:01 p.m.

I dont think thats possible here, getValue() gets the property value from $this->result and get_reflection_property_value() can only get the property value of the reflection when that property is set manualy.

pprkut Sept. 17, 2021, 2:28 p.m.

That's fine. You can update the tests to follow that structure ;)

test.php (Diff revision 7)

Show all issues

We don't need that file :)

Change Summary:

Proccessed feedback

Diff:

Revision 8 (+344)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	tests/phpunit.xml

Change Summary:

Rebase from review #799

Summary:

-	[WIP] [Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries
+	[Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Diff:

Revision 9 (+346)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	tests/phpunit.xml

src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php (Diff revision 9)
Show all issues
```
shouldn't be public, make it protected
```
src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php (Diff revision 9)
Show all issues
```
Superfluous
```
src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php (Diff revision 9)
Show all issues
```
Not correct ;)
```
src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php (Diff revision 9)
Show all issues
```
Superfluous
```
src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php (Diff revision 9)
Show all issues
```
Not correct ;)
```

Change Summary:

Feedback processing

Commit:

-	8bbb991df05cc2e2001410868b23ff3742f55973
+	88776ea7fbee4bb82256db7cb0f9f1f805c1342c

Diff:

Revision 10 (+340)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	tests/phpunit.xml

Change Summary:

rebase

Commit:

-	88776ea7fbee4bb82256db7cb0f9f1f805c1342c
+	502752062b2f83a4e80b02202848fa57f5ab7752

Diff:

Revision 11 (+340)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	tests/phpunit.xml

Ship it!

src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php (Diff revision 11)
Show all issues
```
2021 :)
```
1. b.stoop Sept. 24, 2021, 9:01 a.m.
  It's just 2021 instead of 2012-2021?
2. pprkut Sept. 24, 2021, 9:06 a.m.
  Yes :)
src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php (Diff revision 11)
Show all issues
```
2021 :)
```
src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php (Diff revision 11)
Show all issues
```
2021 :)
```

Change Summary:

Edited @copyright

Commit:

-	502752062b2f83a4e80b02202848fa57f5ab7752
+	96d04bda9726fa49d808b4edca879624552d2022

Diff:

Revision 12 (+340)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLQueryResult.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiSuccessfulWarningConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLiWarning.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLQueryResultWarningTest.php
	tests/phpunit.xml

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Gravity] Integrate mysqli_get_warnings() in the result object for mysql/mariadb queries

Summary:

Description:

Summary:

Testing Done:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Testing Done:

Diff:

Added Files:

Testing Done:

Diff:

Removed Files:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Summary:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: