This change has been marked as submitted.

Describe the submission (optional):

Merged into master

Reviews
Diff

- Download Diff

Summary:

[Gravity]: Add options setter

Review Request #534 — Created Oct. 30, 2017 and submitted 7 years, 5 months ago

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	7 years, 6 months ago
Did you actually test that this is working?	pprkut	7 years, 6 months ago
Might be better to report a status here.	smillernl	7 years, 6 months ago
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	7 years, 6 months ago
Nope	smillernl	7 years, 6 months ago
we still need a way to opt out though	pprkut	7 years, 6 months ago
Besides your choice in programming language for TRUE and False, this is wrong.	smillernl	7 years, 6 months ago
Set the value in the constructor please	pprkut	7 years, 6 months ago
Revert the formatting	pprkut	7 years, 6 months ago
I'd just make it set_option() and make it take key and value. That way we know it's always gonna be ...	pprkut	7 years, 6 months ago
missing $ ;)	pprkut	7 years, 6 months ago
why not check strictly?	smillernl	7 years, 6 months ago
This would just set a singular option right? which is it? - "Set options for the current connection" - "array ...	smillernl	7 years, 6 months ago
why not set_option($key, $value)?	pprkut	7 years, 6 months ago
what are you trying to accomplish here?	pprkut	7 years, 6 months ago
add typing	pprkut	7 years, 6 months ago
There are no open issues

```
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)
The issue has been resolved. Show all issues
```
What does 1 mean here?
```
1. p.valk 7 years, 6 months ago
  set setting true, can also replace it with true if better readable?
src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 2)
The issue has been resolved. Show all issues
```
Might be better to report a status here.
```
1. p.valk 7 years, 6 months ago
  ok, so something as in report false when already connected?

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 2)
The issue has been resolved. Show all issues
```
Did you actually test that this is working?
```
1. p.valk 7 years, 6 months ago
  yes, tested in a prototype and it works.
  will also write unittests for this.
2. pprkut 7 years, 6 months ago
  Hint: The code as is doesn't work
3. p.valk 7 years, 6 months ago
  ah i see it.
4. pprkut 7 years, 6 months ago
  nope, still broken
5. pprkut 7 years, 6 months ago
  nope, still broken
6. p.valk 7 years, 6 months ago
  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)

The issue has been resolved. 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 7 years, 6 months ago

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

smillernl 7 years, 6 months ago

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

src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 4)
The issue has been resolved. 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)
The issue has been resolved. 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)

The issue has been resolved. Show all issues

we still need a way to opt out though

p.valk 7 years, 6 months ago

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

pprkut 7 years, 6 months ago

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)
The issue has been resolved. Show all issues
```
Set the value in the constructor please
```
src/Lunr/Gravity/Database/MySQL/MySQLConnection.php (Diff revision 10)
The issue has been resolved. Show all issues
```
Revert the formatting
```

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

The issue has been resolved. 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)
The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
why not check strictly?
```

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

The issue has been resolved. 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)
The issue has been resolved. 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)

The issue has been resolved. Show all issues

what are you trying to accomplish here?

p.valk 7 years, 6 months ago

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 7 years, 6 months ago

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)
The issue has been resolved. 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

This change has been marked as submitted.

[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: