Replace deprecated Sqlite-calls with PDO-calls #746
Conversation
|
|
||
| $dbfile = 'sqlite:' . $sqllitedir . $name . '.sqlite'; | ||
| if ($this->db = new \PDO($dbfile)) { | ||
| $q = @$this->db->query('SELECT key1 FROM data LIMIT 1'); |
There was a problem hiding this comment.
Error suppression is hardly ever a desirable thing. I know that the original source code had it as well, but shouldn't failures be loud and obvious?
Or better yet, possibly throw an exception like the enclosing if-else condition -- or configure PDO to throw exceptions.
Just food for thought :) It's good to see deprecated things going away.
There was a problem hiding this comment.
I think I agree with you here, however I'm a bit anxious to break behaviour here..
That would mean we have to wait for the 2.0 release before we can merge this.
There was a problem hiding this comment.
Now you've made me wonder if I should use the SimpleSAML\Database class here instead of directly calling PDO...
There was a problem hiding this comment.
At this point I see no easy way to do this, so I'd like to stick with this PR 'as is'.
@thijskh could you please review this?
thijskh
left a comment
There was a problem hiding this comment.
I think this change improves over the current situation, even though there's indeed more ideas for further improvements.
I'm however a bit anxious to merge it without testcases. It seems to me the kind of class that's well-testable with unit tests. Would be good to confirm that way that before and after refactoring produce the same results, and of course to ensure it remains working in the future.
|
I have no clue where to start on that, since I'm totally unfamiliar with writing unit tests. |
|
How does this look @thijskh? |
This commit consists of patches automatically generated for this project on https://scrutinizer-ci.com
|
Very nice work! "Coverage increased (+0.7%) to 29.503%" \o/ One comment: I see that you create the sqlite database but do not remove it from disk at teardown. This is of course fine for travis but leaves cruft on the machine for people running the testsuite locally. |
|
Fixed that! |
Attempt to fix #744
I have tested it with the oauth-module (currently the only module using it) and it's working like a charm!
Should be fully backwards compatible too!