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

--with-mysql deprecated in php 5.6 #28

Closed
daghack opened this issue Oct 24, 2014 · 9 comments
Closed

--with-mysql deprecated in php 5.6 #28

daghack opened this issue Oct 24, 2014 · 9 comments

Comments

@daghack
Copy link
Contributor

daghack commented Oct 24, 2014

This was brought up in #27, and I wanted to open the discussion here, rather than in that particular PR.

@tianon
Copy link
Member

tianon commented Oct 24, 2014

Yeah, I'm -1 on removing it because it's still available and still widely used. If/when upstream removes it completely, I think that warrants removal (since having it in really doesn't hurt people not using it much).

@thaJeztah
Copy link
Contributor

Thanks. I was browsing existing issues and this was actually discusses twice in #6 and #7

Based on those, I think there are two opinions;

  • remove mysql because it is deprecated as of PHP 5.5 docs, (and no longer supported upstream?)
  • keep it in because many people haven't updated their code and still use it

So it really depends; if this is the "official" PHP image, should the image;

  • Advocate "best practice" and not support deprecated extensions, or
  • Go for convenience and make it "just work"

Personally, I'm in favor of best practice. If people have to migrate their code to a newer version of PHP (e.g. 5.3 -> 5.5/5.6) they'll probably already run into issues at some point. Switching to mysqli is just part of that migration.

Additionally, if they really need it, then it's simple to add by downloading the Dockerfile and add a single line (that's a big advantage of Docker).

I think recently, the PHP 5.3 image was dropped because it EOL'd so why treat mysql differently?

However, this is just my 0.02c, and open to other opinions.

@thaJeztah
Copy link
Contributor

Did a bit of reading; To offer people that are still using mysql a way to upgrade; this article on the Oracle site mentions an automated conversion script to migrate existing code; https://github.com/philip/MySQLConverterTool

Of course, people can always opt to stay on the PHP 5.4 image, which still supports mysql

@tianon
Copy link
Member

tianon commented Oct 25, 2014

Solid arguments; I'm convinced. 😄

@thaJeztah
Copy link
Contributor

Well, actually, I've been thinking about it a bit more, and I know I talked about that on another issue; how about this;

  • for PHP5.4, build using --with-mysql
  • for PHP5.5 and higher, build using --with-mysql=shared to build it as a shared extension and disable it by default.

This will more clearly communicate that people should "steer away" from mysql, but are still able to use it by enabling it in their php.ini. This will require some documentation on "how to use it".

Not sure though, if it is problematic to have different behaviour between different versions of the image, wdyt?

To be clear, I have no real objection to "having mysql around", as Tianon said, "if you don't use it, it doesn't hurt", just feel that it's a good thing if official images also try to educate people a bit on best practice.

@md5
Copy link
Contributor

md5 commented Oct 25, 2014

👍 @thaJeztah

@thaJeztah
Copy link
Contributor

I created a PR (#29) to build mysql as a shared extension.

If this is accepted, I think it should be included in the README on the registry; https://registry.hub.docker.com/_/php/

However, I could not find the source of the README in this repository, so was unable to add it.

I tested the change and it seems to work correctly, but having an extra pair of eyes on the change won't hurt to test if it works correctly :)

@yosifkit
Copy link
Member

Most (probably all) of the official images documentation is over here: https://github.com/docker-library/docs.

@tianon
Copy link
Member

tianon commented May 19, 2015

Fixed in #41.

@tianon tianon closed this as completed May 19, 2015
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 a pull request may close this issue.

5 participants