colinmollenhour / Cm_RedisSession

Redis-based session handler for Magento with optimistic locking
208 stars 121 forks source link

Path issue to ConfigInterface.php when using symlink Composer deployment strategy #98

Closed kirkmadera closed 7 years ago

kirkmadera commented 7 years ago

When this module is installed via Composer with the magento-hackathon/magento-composer-installer, the following error occurs:

PHP Fatal error:  require_once(): Failed opening required '/var/www/myproject/branches/master/vendor/colinmollenhour/magento-redis-session/code/Model/../lib/src/Cm/RedisSession/Handler/ConfigInterface.php' (include_path='/var/www/myproject/branches/master/vendor/magento/zendframework1/library:/var/www/myproject/branches/master/app/code/local:/var/www/myproject/branches/master/app/code/community:/var/www/myproject/branches/master/app/code/core:/var/www/myproject/branches/master/lib:.:/usr/share/pear:/usr/share/php') in /var/www/myproject/branches/master/vendor/colinmollenhour/magento-redis-session/code/Model/Session.php on line 32

I am using this as a temporary fix in my composer.json:

    "scripts": {
        "post-install-cmd": [
            "cd vendor/colinmollenhour/magento-redis-session/code/lib && ln -s ../../../php-redis-session-abstract/src"
        ]
    }

There is a missing connection on the lib/src directory needing to be pointed to the php-redis-session-abstract/src directory. Another fix for this I believe would be to not call the require at all in the php and let the autoloader find it.

colinmollenhour commented 7 years ago

Removing require would break installations using modman.. I suppose those could be changed to "include" instead of "require". Let me know if that works for you or if you have a better suggestion that works with both composer and modman.

kirkmadera commented 7 years ago

The use of include rather than require would still give an error (warning instead of fatal); and error suppression is never a good solution. You could use a file_exists conditional. With modman installs, it would exist and would be included. With composer installs, the file wouldn't exist, but would be unnecessary due to the autoloader.

Another solution, but probably more subject to confusion and issues. Since these modules depend on each other, you could add a symlink with the relative path to the src like I am doing in my composer install script.

colinmollenhour commented 7 years ago

I like using file_exists method in this case. The symlink method obviously works, but using post-install-cmd is probably more platform dependent and subject to other issues that would be hard to predict.

Swahjak commented 7 years ago

I think this is related / a duplicate of #96. As I suggested maybe you could make the require statements dependent on 'class_exists'.

if(!class_exists('\Cm\RedisSession\Handler') require ...

This would allow to autoload the classes if possible and otherwise include them. Supporting both symlink / autload related practices and modman installation.

Swahjak commented 7 years ago

On a related note; to allow composer installation we would also need a dependency on credis.

colinmollenhour commented 7 years ago

@Swahjak Credis is already a dependency of the abstract library so should be handled by composer's recursive dependencies.

colinmollenhour commented 7 years ago

Anyone want to test master with composer for me before I tag a new release?

kirkmadera commented 7 years ago

This worked for me this time without needing to add the symlink.

colinmollenhour commented 7 years ago

Cool, I'll make anew release. Thanks for testing!