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

BUG Framework defaults to MySQLi but installer defaults to PDO #8540

Closed
micmania1 opened this issue Oct 29, 2018 · 10 comments
Closed

BUG Framework defaults to MySQLi but installer defaults to PDO #8540

micmania1 opened this issue Oct 29, 2018 · 10 comments

Comments

@micmania1
Copy link
Contributor

Affected Version

4+

Description

When the environment variable SS_DATABASE_CLASS is not set, it will default to MySQLi. However, the installer prefers and defaults to PDO meaning the it has two defaults.

In environments (ie. Prod, UAT) where .env file should by ignored, you have to mindfully set SS_DATABASE_CLASS to PDO, otherwise it will default to MySQLi and you'll be running a different connector in prod vs UAT.

The framework should default to the same as the installer to avoid this issue.

Steps to Reproduce

Set and unset SS_DATABASE_CLASS="MySQLPDODatabase"

@chillu
Copy link
Member

chillu commented Oct 30, 2018

I'd err on the side of caution and say this is a semver break for 4.x. The fact that we've surfaced a tricky issue happening in one driver but not the other already (collation) signifies that there's too much potential for edge cases. Is there a comparison of what PDO does different to mysqli, where developers could judge the risk for themselves?

@micmania1
Copy link
Contributor Author

@chillu not that I know of but maybe @tractorcow can share his wisdom?

@tractorcow
Copy link
Contributor

mysqli is the best default if module detection isn't enabled (framework env failover)
pdo is the best default if module detection is enabled (installer, does that work when making recommendations).

In environments (ie. Prod, UAT) where .env file should by ignored

Those environments shouldn't be ignoring .env should they?

@lerni
Copy link
Contributor

lerni commented Oct 31, 2018

If .env is ignored SS needs a system-user per instance. If it is ignored, it removes flexibility to run multiple SS instances on shared hostings (staging) or makes those "unsharable".

@micmania1
Copy link
Contributor Author

Sorry, its not ignored, but its not committed to the repository. Environment variables are set higher up and passed through apache so that environment specific remains in .env and we prevent accidentally committing sensitive environment credentials.

@tractorcow Can you elaborate a bit more on the MySQLi vs PDO and why one is better than the other in each situation? I was thinking that detection of PDO could be done on each request inexpensively if SS_DATABASE_CLASS isn't set. Can you see any problems with that?

@tractorcow
Copy link
Contributor

tractorcow commented Nov 2, 2018

MySQLi is more likely to be available on a system, so that's why it's the default if we don't do module detection.

However, if both are available, PDO is the better one to use, so if the code is bothering to do the checks it defaults to that.

The real solution here is to just do the module detection in all cases, as you suggest. :)

@micmania1
Copy link
Contributor Author

@tractorcow thank you :)

@sminnee
Copy link
Member

sminnee commented Nov 9, 2018

@tractorcow it’s really for a separate RFC, but in my opinion PDO has been an enormous source of pain far bigger than the effort of maintaining separate Connector implementations and I wonder if we should drop it in SS5 (or move to DBAL)

@ScopeyNZ
Copy link
Contributor

(or move to DBAL)

RFC: #6632

I think this effort would be worth it in the long run so I'll just leave that link there.

@GuySartorelli
Copy link
Member

Installer is effectively abandoned. This is no longer relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants