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

removed submodule for Credis and added as Composer Dependency #70

Closed
wants to merge 1 commit into from
Closed

Conversation

davidverholen
Copy link

resolve #68

you really should not keep ignoring composer. Its used in most big companies to handle many big Projects. I made this change here, and added a modman file in the Credis library.

So now you can still track the new version of the Credis lib, it can be installed via composer and still you can add both with modman and it will still work for you

@colinmollenhour
Copy link
Owner

Thanks for the PR, David. I am all for better composer support, I was just waiting for someone to propose a solution that works without breaking backwards compatibility. :)

I don't see the use of removing the submodule since composer doesn't support submodules so they shouldn't break anything when present. What about instead of removing the submodule replacing the "lib/*" line in the modman file with an "@import lib/Credis" since composer also doesn't support the "import" feature. So these two features will allow it to still work with modman and not cause issues with composer?

The only other issue I see is that I would like to avoid making Credis a "magento-module" since it really is not a magento module and people may already be using it in non-Magento projects (or wish to in the future). So do you see a way to work around this? I don't know all of the composer features myself.. Worst case perhaps there could be a "magento-master" branch for Credis which uses a "magento-module" type for the composer file.

@davidverholen
Copy link
Author

the magento composer installer also has the feature to install libraries i think. never used it, but i will try. I Also don#t like, that the original Credis Client Class is overwritten.

We just had this case this week, that we deleted the module again and that class was not there anymore. It caused some strange behaviour and my colleagues didn't know where to search for the error.

I dont' really have a nice Solution yet.

Replacing the modman entry with an @import should at least avoid composer to delete the original Credis Client ;)

Did you ever test the Backend Cache with the Credis Client shipped in Magento?

Another question, is your new Credis CLient fully backwards compatible?
If the Redis Session Module still works we may just update the file in magento/core

@davidverholen
Copy link
Author

another good solution might be to make the Credis Client psr-0 compliant and use the firegento/psr0autoloader to load it

@colinmollenhour
Copy link
Owner

I have no control over what is shipped with Magento.. So probably the best thing to do would be to change the github/modman version to use app/code/community instead of lib to ensure that the modman version overrides the core version without actually replacing it.

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

Successfully merging this pull request may close these issues.

remove lib/* from modman
2 participants