search

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
869 stars 436 forks source link

Possibility for multiple Credis versions to conflict #1722

Closed justinbeaty closed 1 year ago

justinbeaty commented 3 years ago

Preconditions (*)

  1. OpenMage 20.12 (but I think it will be reproducible in any version)
  2. colinmollenhour/cache-backend-redis installed, and using the version of Cm_RedisSession in this repo.

Steps to reproduce (*)

  1. Install preconditions as noted above, before #1254 was merged, it was impossible to log in

Expected result (*)

  1. Log in in should work as normal

Actual result (*)

  1. With colinmollenhour/cache-backend-redis installed, credis was being loaded from vendor instead of lib/Credis

Discussion

Since #1254 is now merged, I think the more urgent issue is resolved. I've been running the patch from that PR for the past week, and things are okay. However, I am running as headless Magento, and only the admin session creates a redis session. It's possible there are other incompatibilities between the different versions of Credis, but I haven't run into any as far as I know.

Here's the possible different versions that could be installed as I understand it:

So this issue is more of a discussion on how small things like this should be prevented. Perhaps instead of lib/Credis, you would need to install Credis via composer, and the Cm_RedisSession in this repo is updated to support the latest version? Or maybe Cm_RedisSession is removed from this repo and we only should use from composer? Unfortunately, Magento 1.x comes from a time before when composer was popular, and we also have to support people who do not use composer. But, I don't think composer could even have solved this problem, except that it might just warn that you are trying to use two different versions.

justinbeaty commented 3 years ago

I also just re-read Flyingmana's comment in #1705 where it would be best to not involve composer in fixing this, likely because there are still many non-composer users. I think the best fix then is probably updating this repo's version of Cm_Cache_Backend_Redis to work with the latest Credis, and also updating lib/Credis to the latest as well. That way, if one did install Credis via composer (via Cm_Cache_Backend_Redis), then there is no incompatibility. But we would have to wonder if any 3rd party modules rely on the old Credis lib, and updating it would break those modules? I don't have a really good answer yet to all of this so I'm interested to hear other opinions.

Flyingmana commented 3 years ago

related to https://github.com/OpenMage/magento-lts/issues/524 So updating our code, and marking the CM_Redis repository as included in our composer.json may be the easiest.

justinbeaty commented 3 years ago

@Flyingmana I think that can work. We should target 20.x with the updated lib/Credis, since there is the breaking change in the Credis lib. I do not know if other modules may have used Credis directly as well.

I am still not too sure in what case someone would use the Cm_RedisSession in this repo vs the external one. I was using the external one for a while until I decided to use the built in one. Does the external one just have different features?

justinbeaty commented 1 year ago

This should be fixed with v19.5.0-rc1 and v20.1.0-rc1