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

Constructor accepts mixed values which leads to bug #14

Open
lordrhodos opened this issue Feb 21, 2019 · 1 comment
Open

Constructor accepts mixed values which leads to bug #14

lordrhodos opened this issue Feb 21, 2019 · 1 comment

Comments

@lordrhodos
Copy link

lordrhodos commented Feb 21, 2019

Problem

As discussed earlier today the following flow will create a bug

quote from @felixsand earlier in slack today

  1. Initiate a new Db object using a PDO object: $db = new Db($existingPdo);
  2. Now disconnect: $db->disconnect();
  3. What happens now next time $db->connect() is called? No pdo OR dsn is available

Will probably lead to an unexpected Exception of some sort when a new PDO object is trying to get created when just null will be sent in as dsn, username and password, no?

Solution

I suggest the following to solve this issue in the long term:

  • allow only PDO object to be passed into the container in a version 1.0.0
  • deprecate usage of the two other approaches now
  • add static replacement methods to support creating a Db instance by passing in a dns or host, username password and options
  • refactor and the fix the connection issue described above
@lordrhodos lordrhodos changed the title Constructor acception mixed values leads to bug Constructor accepts mixed values which leads to bug Feb 21, 2019
@lordrhodos
Copy link
Author

after looking a little bit deeper into the code and how PDO is handling connections I have to adjust my suggested solution to the following

  • drop support for passing PDO objects into the constructor
  • drop support to pass in a host and a database and let the Db create the dsn
  • support only a PDO dsn string, username (optional) and password (optional)
  • introduce a PdoFactoryInterface (optional) as constructor argument and a default implementation creating the PDO instances

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

No branches or pull requests

1 participant