elastic / elasticsearch-php

Official PHP client for Elasticsearch.
https://www.elastic.co/guide/en/elasticsearch/client/php-api/current/index.html
MIT License
36 stars 970 forks source link

PHP version requirement #10

Closed designermonkey closed 11 years ago

designermonkey commented 11 years ago

This isn't exactly an issue with the codebase here, but I need help in identifying what it is that makes the php version requirement 5.3.9.

I have been made to try and implement this onto a PHP 5.3.3 system, and it fails completely.

Changing the requirement in composer allows composer to install all of the requirements, however it still fails.

I have no error report to use, as Codeigniter doesn't output errors (because it's so wonderful!)

Any helpful advice would be appreciated.

polyfractal commented 11 years ago

This bug is what prompted me to set the version requirement at 5.3.9.

I'm not sure if this particular bug affects the codebase, but I make heavy use of both abstract classes and interfaces, so there is a good chance that I'm using them in combination somewhere like the bug describes. I haven't looked, but some of the dependencies may require >=5.3.9 as well.

designermonkey commented 11 years ago

Thanks for the quick response @polyfractal.

The way I'm doing this is to directly require the vendor/autoload.php file within a Codeigniter controller, and this has worked before but not this time.

I may have to do some digging then as doing this is what causes it to fail.

I'll get back to you on this.

polyfractal commented 11 years ago

Cool cool, keep me updated. If we can rearrange some of the code to satisfy earlier versions, I'd be happy to make the change assuming it doesn't impact the code too much.

designermonkey commented 11 years ago

I've put a mock php file together to do the same implementation but outside of Codeigniter, and I'm getting the following warning, and an otherwise blank page.

Notice: Undefined variable: dicParams in /var/www/html/subdomains/test/public_html/application/libraries/elasticsearch-php/src/Elasticsearch/Common/DICBuilder.php on line 184 Fatal error: Class name must be a valid object or a string in /var/www/html/subdomains/test/public_html/application/libraries/elasticsearch-php/src/Elasticsearch/Common/DICBuilder.php on line 185

designermonkey commented 11 years ago

Just to add, I even tried using double slashes in the class reference strings in that file, but the same error happens.

designermonkey commented 11 years ago

This is also interspersed with chrome giving a No data received error, and Firefox saying the Connection was Reset

I'm completely at a loss here.

polyfractal commented 11 years ago

Hmm, I'm not really sure without taking a closer look. Can you gist up a recreation? Are you trying to inject any parameters into the client (e.g. override the connection pool or something)?

designermonkey commented 11 years ago

Just doing an include and the most basic client call

<?php
    require __DIR__.'application/libraries/elasticsearch-php/vendor/autoload.php';

    $client = new Elasticsearch\Client();
PHP Version 5.3.3
Linux 2.6.32-358.6.2.el6.x86_64
CentOS 6.4
polyfractal commented 11 years ago

Did you install with Composer? I roughly recreated your layout and everything worked fine. Here is what I did:

cd temp
mkdir -p application/libraries

Then create your composer.json in the root folder. Note, I added a special config (vendor-dir) so that the installation path roughly lines up with yours. Default behavior will put it into vendor/ at the current dir:

{
    "config" : {
        "vendor-dir" : "application/libraries"
    },
    "require": {
        "elasticsearch/elasticsearch": "~0.4"
    }
}

Then back to the shell to install everything via composer:

curl -s http://getcomposer.org/installer | php
php composer.phar install

After installation, create your main.php with an include path to autoload.php:

<?php
    require __DIR__.'/application/libraries/autoload.php';

    $client = new Elasticsearch\Client();

Then php main.php and everything works. Here is the full tree layout after installation:

.
├── application
│   └── libraries
│       ├── autoload.php
│       ├── composer
│       ├── elasticsearch
│       ├── guzzle
│       ├── monolog
│       ├── pimple
│       ├── psr
│       └── symfony
├── composer.json
├── composer.lock
├── composer.phar
└── main.php
polyfractal commented 11 years ago

Ah, ignore everything above...my PHPbrew version was incorrect. I'm getting the same error you are on PHP 5.3.3. Will begin to investigate if it is fixable or not given the version.

designermonkey commented 11 years ago

Thanks for looking at this.

polyfractal commented 11 years ago

So I tracked down the issue, and I'm afraid PHP 5.3.9 is definitely the minimum version.

This bug prevents closures from working if they implement ArrayAccess. Pimple (the dependency injection container) makes heavy use of ArrayAccess, and in certain instances I need to close over the Pimple object itself to return a new function. For example:

private function setConnectionObj()
{
    $this->dic['connection'] = function ($dicParams) {
        return function ($host, $port = null) use ($dicParams) {   // This is the problem
            return new $dicParams['connectionClass'](
                $host,
                $port,
                $dicParams['connectionParamsShared'],
                $dicParams['logObject'],
                $dicParams['traceObject']
            );
        };
    };
}

Here, I'm setting a Pimple property ('connection') equal to a function, but the function uses a closure to return a new function. I'm doing this so I can avoid more verbose Factory classes all over the place. You can see that the returned function is a closure that captures $dicParams, and since $dicParams is actually a Pimple object which implements ArrayAccess, we run into the bug.

If you upgrade to 5.3.7, that particular bug is fixed but we start to run into the bug that I mentioned before regarding abstract classes and interfaces:

$ phpbrew use php-5.3.7
$ php main.php

PHP Fatal error:  Can't inherit abstract function Elasticsearch\Connections\ConnectionInterface::getTransportSchema() (previously declared abstract in Elasticsearch\Connections\AbstractConnection) in /Users/tongz/Documents/temp/application/libraries/elasticsearch/elasticsearch/src/Elasticsearch/Connections/AbstractConnection.php on line 26

Fatal error: Can't inherit abstract function Elasticsearch\Connections\ConnectionInterface::getTransportSchema() (previously declared abstract in Elasticsearch\Connections\AbstractConnection) in /Users/tongz/Documents/temp/application/libraries/elasticsearch/elasticsearch/src/Elasticsearch/Connections/AbstractConnection.php on line 26

So it looks like PHP 5.3.9 really is the absolute minimum. You'll have to upgrade to use the library, sorry. :( I can't see any way to fix these issues without doing extreme remodeling of the internals...and even then I'm not sure we wouldn't run into the bug elsewhere. These are bugs in some very basic PHP functionality (closures, abstract classes and interfaces).

designermonkey commented 11 years ago

Thanks for spending the time to find this, I was lost.

I will use that explanation to help get the site migrated onto a more up to date server.

caomania commented 10 years ago

May I suggest adding the PHP 5.3.9 version requirement more prominently in the readme?

polyfractal commented 10 years ago

@caomania Very good idea, I'll do that as soon as possible (in the airport at the moment).

Neeke commented 10 years ago

thanks a lot