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

Use DatabasePlatform instead of Driver to check for MySQL/MariaDB #1984

Merged
merged 1 commit into from
Oct 13, 2020
Merged

Use DatabasePlatform instead of Driver to check for MySQL/MariaDB #1984

merged 1 commit into from
Oct 13, 2020

Conversation

andysh-uk
Copy link
Contributor

@andysh-uk andysh-uk commented Oct 12, 2020

Replace getDriver()->getName() call with getDatabasePlatform()->getName() to avoid a deprecation in Doctrine.

This returns mysql instead of pdo_mysql on MariaDB, at least, so the mb_strpos check should work just fine as-is.

Copy link
Member

@bobdenotter bobdenotter left a comment

Choose a reason for hiding this comment

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

Thank you, @andysh-uk for this fix! Also thanks to you @Wieter, for helping to get to the root of the problem!

I love fixes like this! It seems like such an insignificant change, since it literally only changes one method call. The previous fix was a patch, fixing the symptoms. This little one-word change took several hours of work from multiple people, however it is a proper fix for an actual bug, and reduces workarounds further down the line! 🤗

@Wieter
Copy link
Contributor

Wieter commented Oct 13, 2020

@bobdenotter you're welcome :)

This fix prevents future breakage due to depreciation, however it does not resolve issues #1971 and #1985 (outdated config file as introduced by composer install).

@andysh-uk
Copy link
Contributor Author

I love your sentiments @bobdenotter (thank you) however the credit goes to @Wieter for finding the actual issue (with doctrine.yaml) - as you said, my previous fix was incorrect as it only worked around a problem, not resolved the root of the problem.

This PR is just something I noticed whilst working on this issue, that the Doctrine project have deprecated this method, believing it only to be used in their tests.

@bobdenotter bobdenotter merged commit c861215 into bolt:master Oct 13, 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.

3 participants