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

Database independent PDO checks #655

Closed
Silic0nS0ldier opened this issue Feb 22, 2017 · 4 comments
Closed

Database independent PDO checks #655

Silic0nS0ldier opened this issue Feb 22, 2017 · 4 comments
Labels
compatibility Compatibility issue with other framework, features confirmed bug Something isn't working
Milestone

Comments

@Silic0nS0ldier
Copy link
Member

Currently there is code intended to trigger a PDOException on database connection failure, due to built in library throwing a useless exception that makes debugging next to impossible.

However this check is specific to MySQL (and by extension MariaDB) as PDO expects different keywords for different database systems.

This check should be refactored into a class that mimics the behaviour of Laravels library, or a fix should be made to their code so that a more debugable exception is thrown. (assuming their latest version isn't above our minimum PHP requirement, it shouldn't be from memory)

@Silic0nS0ldier Silic0nS0ldier added compatibility Compatibility issue with other framework, features confirmed bug Something isn't working V4 labels Feb 22, 2017
@alexweissman alexweissman modified the milestone: 4.0 Mar 1, 2017
@alexweissman
Copy link
Member

So, I just discovered this commit to illuminate/database 5.3: laravel/framework#16666

Which references this issue: laravel/framework#16661

Notice that this fix happened only two days after I implemented our own check!.

So, if I'm understanding this correctly, I think we can completely remove our database check, and just let Laravel throw the actual PDOException now if the connection fails.

@lcharette
Copy link
Member

lcharette commented Mar 20, 2017

@alexweissman
Copy link
Member

So, I think we can just remove the check from Authenticator completely, and use getPdo() to test the connection in the installer now.

@alexweissman
Copy link
Member

Alright, implemented in 9c9316b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility issue with other framework, features confirmed bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants