Pdo sqlite keep bc since php81 #320
Conversation
hungtrinh
commented
Feb 6, 2023
•
hungtrinh
commented
- Carries [github-actions] enable SQLite tests zf1s/zf1#157 which was created by @partikus (keep BC for sqlite adapter when php >=8.1)
- Github action, sticky with phpunit 9 (now phpunit 10 is latest version on github action, zf1-future support phpunit 9 only)
|
Mr @glensc could you please review this PR, thanks you! |
|
@hungtrinh can you move phpunit change to separate PR? |
|
Waiting #321 merged then rebase from master, so back PR to Draft |
hungtrinh
force-pushed
the
pdo-sqlite-keep-bc-since-php81
branch
from
604a401
to
e158212
Compare
|
@hungtrinh you should apply all my suggestions. i.e to use short array. |
|
Yep, i will apply it |
hungtrinh
force-pushed
the
pdo-sqlite-keep-bc-since-php81
branch
2 times, most recently
from
64f3404
to
4311fb9
Compare
|
Rebase from master. Applied all suggestions. |
|
Can you squash the suggestions to single commit and put meaningful commit message there (Use short array and coalesce language features)? |
|
Yep let me do it |
hungtrinh
force-pushed
the
pdo-sqlite-keep-bc-since-php81
branch
from
4311fb9
to
633163b
Compare
It's done Mr @glensc |
| @@ -269,7 +269,7 @@ public function testAdapterQuoteNullByteCharacter() | |||
| public function testAdapterZendConfigEmptyDriverOptions() | |||
| { | |||
| $params = $this->_util->getParams(); | |||
| $params['driver_options'] = ''; | |||
| $params['driver_options'] = null; | |||
There was a problem hiding this comment.
why null? looking at other code in this PR, looks logical to use []
There was a problem hiding this comment.
Yep look logical to use []
why
nullnot keep empty string''?
To verify case use Null Coalescing Operator after apply your suggestion
library/Zend/Db/Adapter/Pdo/Sqlite.php
- $config['driver_options'] = empty($config['driver_options']) ? array() : $config['driver_options'];
+ $config['driver_options'] = $config['driver_options'] ?? [];There was a problem hiding this comment.
[] would work for ?? as well.
There was a problem hiding this comment.
Can you make the change then? :)
There was a problem hiding this comment.
Can you make the change then? :)
it's done Mr @glensc
Just for fun:
Before replace empty string '' by null in test case, i predicted we would have a conversation about it, i am a prophet :)
There was a problem hiding this comment.
@hungtrinh that is good. no way can slip in extra hidden changes! you should have committed it separately with an explanation in the commit message body, then wouldn't have to discuss it in the review notes. i.e more transparent change. because it doesn't match the commit message it's committed under now.
There was a problem hiding this comment.
Mr @glensc please merge this PR
There was a problem hiding this comment.
I was hoping you split the commit. I guess not then.
Co-authored-by: Elan Ruusamäe <glen@pld-linux.org>
hungtrinh
force-pushed
the
pdo-sqlite-keep-bc-since-php81
branch
from
633163b
to
e5fc4ea
Compare
