- Download Diff

Summary:

[Gravity]: Add options setter

Review Request #534 — Created Oct. 30, 2017 and submitted Nov. 10, 2017, 11:07 a.m.

Information
Owner:	p.valk
Repository:	Lunr
Branch:	Gravity_options
Bugs:
Depends On:
Blocks:	535
Commit:	d6cbfeb...
Reviewers
Groups:	lunr
People:

Description

Gravity: Add options setter

Testing Done

unittests.

Issues

0
0
16
0
All Issues: 16

Description	From	Last Updated
What does 1 mean here?	smillernl	Nov. 1, 2017, 11:53 a.m.
Did you actually test that this is working?	pprkut	Nov. 1, 2017, 4:02 p.m.
Might be better to report a status here.	smillernl	Nov. 1, 2017, 11:53 a.m.
This would suggest [MYSQLI_OPT_INT_AND_FLOAT_NATIVE, MYSQLI_OPT_INT_AND_FLOAT_NATIVE] is a correct usage of the function but the code suggests it isn't.	smillernl	Nov. 1, 2017, 3:07 p.m.
Nope	smillernl	Nov. 1, 2017, 2:16 p.m.
we still need a way to opt out though	pprkut	Nov. 1, 2017, 5:28 p.m.
Besides your choice in programming language for TRUE and False, this is wrong.	smillernl	Nov. 1, 2017, 5:28 p.m.
Set the value in the constructor please	pprkut	Nov. 1, 2017, 5:38 p.m.
Revert the formatting	pprkut	Nov. 1, 2017, 5:38 p.m.
I'd just make it set_option() and make it take key and value. That way we know it's always gonna be ...	pprkut	Nov. 1, 2017, 5:38 p.m.
missing $ ;)	pprkut	Nov. 1, 2017, 5:36 p.m.
why not check strictly?	smillernl	Nov. 1, 2017, 5:51 p.m.
This would just set a singular option right? which is it? - "Set options for the current connection" - "array ...	smillernl	Nov. 1, 2017, 5:51 p.m.
why not set_option($key, $value)?	pprkut	Nov. 6, 2017, 9:50 a.m.
what are you trying to accomplish here?	pprkut	Nov. 6, 2017, 11:57 a.m.
add typing	pprkut	Nov. 6, 2017, 1:40 p.m.

```
LGTM
```

Commit:

-	9779a9bb1e6d9d0923b14139532db23c7ce8ac89
+	08c4412423d01b7b7f9f02cd17dd273ba65e11cf

Diff:

Revision 2 (+27)

Show changes

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

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 2)
Show all issues
```
What does 1 mean here?
```
1. p.valk Nov. 1, 2017, 10:57 a.m.
  set setting true, can also replace it with true if better readable?
src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 2)
Show all issues
```
Might be better to report a status here.
```
1. p.valk Nov. 1, 2017, 10:57 a.m.
  ok, so something as in report false when already connected?

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 2)
Show all issues
```
Did you actually test that this is working?
```
1. p.valk Nov. 1, 2017, 11:28 a.m.
  yes, tested in a prototype and it works.
  will also write unittests for this.
2. pprkut Nov. 1, 2017, 11:36 a.m.
  Hint: The code as is doesn't work
3. p.valk Nov. 1, 2017, 11:51 a.m.
  ah i see it.
4. pprkut Nov. 1, 2017, 1:12 p.m.
  nope, still broken
5. pprkut Nov. 1, 2017, 2:21 p.m.
  nope, still broken
6. p.valk Nov. 1, 2017, 2:47 p.m.
  what is still broken about it now?

Commit:

-	08c4412423d01b7b7f9f02cd17dd273ba65e11cf
+	aba65244590c302caa473978d036923818e07c53

Diff:

Revision 3 (+29)

Show changes

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

Commit:

-	aba65244590c302caa473978d036923818e07c53
+	411c04e8e4fa445f3e3c14f8f84dcfe47111dbbb

Diff:

Revision 4 (+29)

Show changes

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

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

Show all issues

This would suggest [MYSQLI_OPT_INT_AND_FLOAT_NATIVE, MYSQLI_OPT_INT_AND_FLOAT_NATIVE] is a correct usage of the function but the code suggests it isn't.

p.valk Nov. 1, 2017, 2:21 p.m.

do we have a way to tell its a key value array or should i just add an example in commemt?

smillernl Nov. 1, 2017, 2:30 p.m.

You could use more words to describe the type of array.

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 4)
Show all issues
```
Nope
```

Commit:

-	411c04e8e4fa445f3e3c14f8f84dcfe47111dbbb
+	b91ff9622e57c96edd4832164cf679fd23f422f7

Diff:

Revision 5 (+29)

Show changes

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

Commit:

-	b91ff9622e57c96edd4832164cf679fd23f422f7
+	ce16ea32b97b327d652aa95d0aab2ad78a20f265

Diff:

Revision 6 (+29)

Show changes

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

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

Show all issues

Besides your choice in programming language for TRUE and False, this is wrong.

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

Show all issues

we still need a way to opt out though

p.valk Nov. 1, 2017, 4:21 p.m.

well just call $this->set_options([MYSQLI_OPT_INT_AND_FLOAT_NATIVE => FALSE]);

pprkut Nov. 1, 2017, 4:29 p.m.

sure, that's fine. But we need to be able to do that from the locator recipe

Commit:

-	ce16ea32b97b327d652aa95d0aab2ad78a20f265
+	9c0f0fd4352858d05b7f00d36877259e5da8c893

Diff:

Revision 7 (+29)

Show changes

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

Commit:

-	9c0f0fd4352858d05b7f00d36877259e5da8c893
+	4f89c802155947d7488b3948f993f3a96e514b4f

Diff:

Revision 8 (+41 -4)

Show changes

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

Commit:

-	4f89c802155947d7488b3948f993f3a96e514b4f
+	78567417c5d3d4c5b954ab8030ce8fb8f756868c

Diff:

Revision 9 (+42 -4)

Show changes

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

Commit:

-	78567417c5d3d4c5b954ab8030ce8fb8f756868c
+	42925086ea21a4369a8dfdc84606deb8f093b216

Diff:

Revision 10 (+33 -3)

Show changes

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

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 10)
Show all issues
```
Set the value in the constructor please
```
src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 10)
Show all issues
```
Revert the formatting
```

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 10)

Show all issues

I'd just make it set_option() and make it take key and value. That way we know it's always gonna be an array

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 10)
Show all issues
```
missing $ ;)
```

Commit:

-	42925086ea21a4369a8dfdc84606deb8f093b216
+	969cb2679fece79a028d67750064c71cdbfc8b1f

Diff:

Revision 11 (+31)

Show changes

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

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 11)
Show all issues
```
why not check strictly?
```

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 11)

Show all issues

This would just set a singular option right?

which is it?
- "Set options for the current connection"
- "array $options"
- "set_option($options)"

Commit:

-	969cb2679fece79a028d67750064c71cdbfc8b1f
+	f1c8fe9cda1754f0537b1b9389f3033585784f75

Diff:

Revision 12 (+31)

Show changes

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

Commit:

-	f1c8fe9cda1754f0537b1b9389f3033585784f75
+	f097853b34cd74267eddee3435e233904567de98

Diff:

Revision 13 (+31)

Show changes

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

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 13)
Show all issues
```
why not set_option($key, $value)?
```

Commit:

-	f097853b34cd74267eddee3435e233904567de98
+	0edd60e117d4fd1cb422615f74c816c7f08ccbe7

Diff:

Revision 14 (+32)

Show changes

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

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 14)

Show all issues

what are you trying to accomplish here?

p.valk Nov. 6, 2017, 10:12 a.m.

protection to see if the values are set/not empty.

and can't use empty() check because the value could be false. (key could be checked with empty())

pprkut Nov. 6, 2017, 10:23 a.m.

Well, the values are set, because if they are not, PHP errors with a missing argument error.
Which leaves invalid values. The key check can be simplified is !is_int(). The value check IMHO isn't really necessary, but if you insist you should just only do a NULL check.

Commit:

-	0edd60e117d4fd1cb422615f74c816c7f08ccbe7
+	206301f49d40763e9a9d5f9310e74842186e7667

Diff:

Revision 15 (+80)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLi.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLConnectionBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLConnectionConnectTest.php

Ship it!

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 15)
Show all issues
```
add typing
```

Commit:

-	206301f49d40763e9a9d5f9310e74842186e7667
+	d6cbfeb347c832797a736c5d79676dae267fd4e4

Diff:

Revision 16 (+80)

Show changes

	src/Lunr/Gravity/Database/MySQL/MySQLConnection.php
	src/Lunr/Gravity/Database/MySQL/Tests/MockMySQLi.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLConnectionBaseTest.php
	src/Lunr/Gravity/Database/MySQL/Tests/MySQLConnectionConnectTest.php

Testing Done:

~		unittests still missing.
	~	unittests.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Gravity]: Add options setter

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Testing Done:

Status: Closed (submitted)

Change Summary: