etsy / opsweekly

On call alert classification and reporting
MIT License
761 stars 100 forks source link

Deprecate use of mysqli_result::fetch_all #33

Closed jaxxstorm closed 9 years ago

jaxxstorm commented 9 years ago

I'm getting the following error while trying to install:

PHP Fatal error:  Call to undefined method mysqli_result::fetch_all() in /var/www/html/opsweekly/phplib/base.php on line 213

Any idea what I've done wrong?

lozzd commented 9 years ago

You need to install the PHP native MySQL driver, mysqlnd. Depending on your operating system, this should be as easy as installing the package from your package manager and restarting your web server.

Lemme know if this helps!

jaxxstorm commented 9 years ago

Hi lozzd, the driver should already be installed, afaik:

# rpm -qa | grep -i php
php-common-5.3.3-40.el6_6.x86_64
php-mysql-5.3.3-40.el6_6.x86_64
php-pdo-5.3.3-40.el6_6.x86_64
php-5.3.3-40.el6_6.x86_64
php-cli-5.3.3-40.el6_6.x86_64
lozzd commented 9 years ago

Is it enabled? Create a test.php with phpinfo(); in it, and see if the mysqli stuff shows up.

You should have a line in a php.ini somewhere that says extension=mysqli.so

jaxxstorm commented 9 years ago

Yes, it seems to enabled:

# php -m | grep -i mysql
mysql
mysqli
pdo_mysql
# cat info.php
<?php
if (!extension_loaded('mysqli')) {
    if (!dl('mysqli.so')) {
    print "mysql extension not loaded";
        exit;
    }
} else {
  print "mysql extension loaded fine\n";
}
?>
[root@host html]# php info.php
mysql extension loaded fine
jaxxstorm commented 9 years ago

I found the problem. The php native mysql is required, which the default CentOS php isn't compiled with.

Even when I enable php with the native mysql driver, I get an error at the top of the page like so:

MySQL support is requiredMySQL support is required

I think this is a major issue for most deployments, so I would really recommend not using the particular function call there.

Relevant links: http://stackoverflow.com/questions/11664536/fatal-error-call-to-undefined-method-mysqli-resultfetch-all http://stackoverflow.com/questions/13159518/how-to-enable-mysqlnd-for-php http://php.net/manual/en/mysqli-result.fetch-all.php

bradbonkoski commented 9 years ago

I have actually had the same problem, and with Centos still being so far behind in terms of PHP (Even Centos 7 is only php 5.4), I have a port which uses PDO as opposed to the native drivers which is nice in that it does 2 things:

  1. Makes it work for us stuck in the Centos world of php 5.3
  2. Also makes it work when connecting to MariaDB for example.

I could try to clean up the port a bit and submit a pull request, but based on the architecture it's not a trivial change. Is this something the primary maintainers would be interesting in merging in?

-brad

On Wed, Dec 3, 2014 at 7:28 PM, Lee Briggs notifications@github.com wrote:

I found the problem. The php native mysql is required, which the default CentOS php isn't compiled with.

Even when I enable php with the native mysql driver, I get an error at the top of the page like so:

MySQL support is requiredMySQL support is required

I think this is a major issue for most deployments, so I would really recommend not using the particular function call there.

Relevant links:

http://stackoverflow.com/questions/11664536/fatal-error-call-to-undefined-method-mysqli-resultfetch-all http://stackoverflow.com/questions/13159518/how-to-enable-mysqlnd-for-php

— Reply to this email directly or view it on GitHub https://github.com/etsy/opsweekly/issues/33#issuecomment-65471822.

jaxxstorm commented 9 years ago

I've updated the title to reflect the new requirement, hopefully that's ok. @bradbonkoski

Did you fork the code? Can I have a look, I might be able to contribute

bradbonkoski commented 9 years ago

Here is the fork I have and that is currently in use. Again, some of the stuff could be cleaned up a bit if it's something that would be considered for merging..

https://github.com/shazam/opsweekly

On Wed, Dec 3, 2014 at 7:37 PM, Lee Briggs notifications@github.com wrote:

I've updated the title to reflect the new requirement, hopefully that's ok. @bradbonkoski https://github.com/bradbonkoski

Did you fork the code? Can I have a look, I might be able to contribute

— Reply to this email directly or view it on GitHub https://github.com/etsy/opsweekly/issues/33#issuecomment-65473467.

jaxxstorm commented 9 years ago

That works absolutely perfectly for me Brad, I'll see if I can help getting it cleaned up

lozzd commented 9 years ago

I'm kinda stuck here, because we upgraded this to mysqli because of deprecation warnings in house and in things like Ubuntu, whereas CentOS is so far behind you folk are having issues.

However, according to @rlerdorf it would be easier to just swap out fetch_all with fetch_array and loop on it (he was the one that upgrade the code to use mysqli, which is most commonly used with mysqlnd)

jaxxstorm commented 9 years ago

I think considering the amount of people running SOME form of CentOS, it'd be ideal to have some support for it.

lozzd commented 9 years ago

Let me see if I can make this work and remove fetch_all.

rlerdorf commented 9 years ago

I am confused. Why not just do:

https://gist.github.com/d414104cb587118b42e1

It has been a long time since I ran across a PHP build that used mysqli linked against libmysqlclient but that simple fix should address it.

jaxxstorm commented 9 years ago

I can confirm that @rlerdorf's patch helps for me

bradbonkoski commented 9 years ago

Even on my dev box running Ubuntu with php 5.5.9 the PDO version works nicely, I guess that's why I lean towards PDO (and I can connect without warnings to mariadb). Which is why I forked, I'm sure my approach is one of many ways to solve the problem..

lozzd commented 9 years ago

If you wish to maintain a PDO fork I can't imagine you would have too much trouble keeping it up to date. I personally dislike PDO and prefer the cleaner native mysql bits.

@jaxxstorm thanks for confirming. I will make that update (or feel free to make a pull request and I'll merge it if you have it handy)

dobber commented 9 years ago

I had the same problem with my ubuntu and thought that I might be wrong somewhere so I didn't commit a patch. My fix looks absolutly like @rlerdorf code, so confirmed to work

lozzd commented 9 years ago

@dobber Do you have a pull request please? :D

lozzd commented 9 years ago

Okay, so that should now be fixed. fetch_all has been removed. Can everyone test and confirm we're good (sorry, still not PDO)