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

PHP version requirement #10

Closed
designermonkey opened this issue Oct 11, 2013 · 16 comments
Closed

PHP version requirement #10

designermonkey opened this issue Oct 11, 2013 · 16 comments

Comments

@designermonkey
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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

@designermonkey
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

Thanks for looking at this.

@polyfractal
Copy link
Contributor

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
Copy link
Author

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
Copy link

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

@polyfractal
Copy link
Contributor

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

@Neeke
Copy link

Neeke commented Jul 23, 2014

thanks a lot

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

No branches or pull requests

4 participants