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

Drop PHP 5.3 from Travis-CI build matrix #880

Merged
merged 4 commits into from
Sep 5, 2015

Conversation

billschaller
Copy link
Member

Drops php 5.3 from travis build matrix and updates composer.json to require php >= 5.4

@billschaller billschaller changed the title drop php 5.3 from travis build matrix drop php 5.3 support Jul 15, 2015
@Ocramius Ocramius self-assigned this Jul 15, 2015
@deeky666
Copy link
Member

👍 as long as you cleaned all 5.3 stuff I'm okay with it. Just for a reference:

doctrine/orm#1255

this PR also optimized some things like $this in closures and stuff.

@billschaller
Copy link
Member Author

@deeky666 as far as I can tell there's no similar changes needed to dbal - no issues with $this in closures, no conditional blocks checking PHP_VERSION_ID other than 50600, etc.

@Tobion
Copy link
Contributor

Tobion commented Aug 29, 2015

@zeroedin-bill there is a reference to PHP 5.3 in https://github.com/doctrine/dbal/blob/master/docs/en/reference/security.rst#non-ascii-compatible-charsets-in-mysql
I guess this paragraph can be removed. Otherwise this PR looks good. 👍

@billschaller
Copy link
Member Author

@Tobion This means that people who expect to use non-ascii charsets with MySQL MUST use the charset parameter, right? So essentially the paragraph should remain, but the bit about PHP < 5.3.6 should be removed?

@Tobion
Copy link
Contributor

Tobion commented Aug 30, 2015

No, from what I understand this is only relevant for PHP <= 5.3.6. Using a newer version should fix the charset problem automatically using SET NAMES, which is what doctrine does.

@billschaller
Copy link
Member Author

@Tobion

Up until PHP 5.3.6 PDO has a security problem when using non ascii compatible charsets. Even if specifying the charset using "SET NAMES", emulated prepared statements and PDO#quote could not reliably escape values, opening up to potential SQL injections. If you are running PHP 5.3.6 you can solve this issue by passing the driver option "charset" to Doctrine PDO MySQL driver. Using SET NAMES does not suffice!

To me this seems to imply that if you are running PHP > 5.3.6, and you do not set the charset, you are still open to injection. I don't use MySQL regularly, so I'm not up on the standard practice here... If I don't set the charset, but I am using emulated prepared statements, is it possible to inject using UTF-8 characters in parameter values?

@Tobion
Copy link
Contributor

Tobion commented Sep 3, 2015

What this describes is the workaround for Up until PHP 5.3.6 PDO has a security problem when using non ascii compatible charsets.. So you don't need it if you use a newer php version.

@billschaller
Copy link
Member Author

@Tobion OK, thanks.

@deeky666
Copy link
Member

deeky666 commented Sep 5, 2015

@Ocramius ready to merge from my POV. Anything missing here? Otherwise you can mergie :)

Ocramius added a commit that referenced this pull request Sep 5, 2015
@Ocramius Ocramius merged commit 251f9e7 into doctrine:master Sep 5, 2015
@Ocramius
Copy link
Member

Ocramius commented Sep 5, 2015

@deeky666 done :)

@Ocramius
Copy link
Member

Ocramius commented Sep 5, 2015

Note: created http://www.doctrine-project.org/jira/browse/DBAL-1292 to track this.

@deeky666
Copy link
Member

deeky666 commented Sep 5, 2015

@Ocramius good idea. thx!

@Ocramius Ocramius removed the BC Break label Jul 22, 2017
@Ocramius Ocramius changed the title drop php 5.3 support Drop PHP 5.3 support Jul 22, 2017
@Ocramius Ocramius changed the title Drop PHP 5.3 support Drop PHP 5.3 from Travis-CI build matrix Jul 22, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants