Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing with PDO backend. Fixes #9 #10

Closed
wants to merge 21 commits into from
Closed

Implementing with PDO backend. Fixes #9 #10

wants to merge 21 commits into from

Conversation

rimas-kudelis
Copy link

@rimas-kudelis rimas-kudelis commented Mar 25, 2020

This (hopefully) fixes #9, albeit with some caveats. Quoting the README, here are the missing features:

  • Support for last insert id. The pdo-firebird extension doesn't support this natively. You implemented it by using a query, but it requires the name of a sequence as an argument. Not sure if implementing it in PDO this way would be much of a win over not implementing it at all. Futhermore, I'm not sure if it works within a transaction (I think it was failing for me with a "transaction is already active" or similar message, but that was before I actually fixed transactions, so it's probably not an issue anymore.
  • Support for different transaction isolation levels. It appears that to implement this, we'd have to handle transactions on our side without relying on PDO. Your original Connection class was mostly about it, but this was a bit too much for me, so I decided to just go with the default isolation level for now.
  • Transaction lock timeouts and waiting. Same issues as above.
  • Autocommitting changes from last queries during object destruction, if they were not made within an explicit transaction. Again, the test just kept failing and both my patience and time were running out. But honestly, I'm just not sure why it wouldn't work out of the box when autocommit is on.
  • Quoting queries without calling the constructor first. That's just not how PDO operates, and likely for a good reason.
  • Generating and returning a DSN. This is a protected method of the driver now.
  • Sanitization of options passed to the driver. I just rely on PDO to do the right thing.
  • Support for Firebird SQL dialects. The underlying PDO driver claims to support dialects 1 and 3, but tests for dialect 1 just kept failing, even though the DSN I generated did include dialect version. I ended up just deleting the test. ext-pdo-firebird only supports dialects starting with PHP version 7.4, so this driver will also only support it starting on PHP 7.4 and up. Futhermore, only dialects 1 and 3 are supported according to PHP documentation, so even though the test for dialect 2 passes, I'm not sure it always will. And dialect 0? I guess there's no such thing. The test for dialect 0, which expected it to be treated as 3, failed, so I assume in PDO it falls back to 1 instead of 3. Either way, I guess it shouldn't be used or tested against.

I also removed some assertions about internal class architecture from the tests. Swapping the backend ruined those assertions. I think we're supposed to test how the unit works, as opposed to how it is constructed.

kafoso and others added 18 commits November 2, 2018 23:15
- Adhere better to PSR-1 and PSR-2.
- Some logical issues were fixed.
- Utilized PHP 7.1 syntax, e.g. strict types.
Now all run in separate processes.
- Cf. issue #6.
- `Connection` constructor now correctly applies the desired dialect.
- Added tests targeting know cases for differences in dialects.
- Refactored `AbstractIntegrationTest` to allow additional customization of the initialization of the connection, installation of the database, and creation of the `EntityManager`.
@rimas-kudelis
Copy link
Author

rimas-kudelis commented Mar 25, 2020

BTW I also merged your 2.7 branch back into master.
Also, 52cf95a might be worth cherry-picking into 2.7 as well, since you broke symlinks you had committed previously. Although I don't think you should even commit them: they seem to be created automatically during composer install.

@rimas-kudelis
Copy link
Author

The PR was closed because I deleted my fork, but I think you can still reopen it if you feel like it.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The driver is incompatible with PHP 7.4
3 participants