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

adds support for insert id for postgres #17

Conversation

developernaren
Copy link
Contributor

Adds support for insert id in postgres

Changes made

  • For some reason the composer.lock does not have phpunit so did a composer install to update the composer
  • Changed the container names to be more drift specific because it was conflicting with my containers in my local docker
  • Extracted insert for all the drivers into their own class

@developernaren
Copy link
Contributor Author

Circle Ci check is failing the tests. I had the same issue when I first ran the tests. It gave me an error table test.test does not exits. The I looked around in the test code. I changed the resetInfrastructure to createInfrastucture it worked and it started working in other consecutive test runs. I am not exactly sure what the issue is.

@@ -21,7 +21,7 @@
/**
* Class EmptyDoctrineMysqlDriver.
*/
final class EmptyDoctrineMysqlDriver extends AbstractMySQLDriver
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the meaning of that removed line

@mmoreram
Copy link
Member

Tests are not passing

  • I'm not sure why EmptyDoctrineMysqlDriver should stop extending AbstractMySQLDriver
  • With this new implementation, when inserting an item in a non existing table, tests fail, basically because the query have RETURNING " without fields.
  • Once this last thing is solved, then tests are failing on line 556 - Failed asserting that null matches expected 1. - and sometimes the line 326

I'm fixing all these problems and I will update the PR and ping you.

Thanks!


abstract class AbstractDriver
{
abstract public function query(string $sql, array $parameters): PromiseInterface;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is already defined for implementation in the Driver interface. Because this class is abstract, you don't have to implement it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all drivers have to extend this AbstractDriver, then the interface can be implemented directly in this abstract class and not in the specific classes (the contract is implicit when you extend)

@mmoreram
Copy link
Member

@developernaren I've completed your PR here - #19

@mmoreram mmoreram closed this Jun 16, 2020
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.

2 participants