Move phpseclib, mcrypt_compat, net_idna2, Cm_RedisSession and Cm_Cache_Backend_Redis to composer

[fballiano]

I think it should be time to start cleaning our repo from 3rd party modules (zend framework anyone?) but I'd like to start from a small a simpler one (at least to me).

Well, we could remove Cm_RedisSession and leave it up to the user to install it (as any other module) from @colinmollenhour's repo.

Or, with the PR, I'm removing it from the repo and moving it to composer.json.

For better discussing the matter I'll start a comment with some observation.

[fballiano]

Another important thing, we can't release something like this if we don't have a "release builder" that creates the "release.tgz" downloading the module and adding it to "app/code", right? how could we approach that? I could create a script for that.

I've searched github documentation but it seems to me that there's no automatic way to include some build binaries into a newly created release, am I missing something?

[justinbeaty]

I’m a fan of this as well as the general direction of using composer modules and a build step.

If this direction is taken, we should remove lib/credis and have that added via the build script as well. There is some more details in #1722 that I encountered last year.

[fballiano]

I've also moved zf1 (introducing zf1-future) and pelago_emogrifier to composer.

From what I've tested it work flawlessly

[tmotyl]

The problem with that is that many people still runs magento in non-composer mode.
For them we need to provide an easy to use way of downloading magento with dependencies.
This can be achieved by building and attaching a zip with full openmage to every release.

This way we could further optimize Magento -> e.g. in composer version rely on composer autoloader and patch Mage.php only for "traditional" build.

[fballiano]

The problem with that is that many people still runs magento in non-composer mode. For them we need to provide an easy to use way of downloading magento with dependencies. This can be achieved by building and attaching a zip with full openmage to every release.

I also run every customer without composer, it's fundamental to have a release builder, do you know some specific github instruments to do so? cause actually i didn't find much.

[fballiano]

I was thinking, why don't we commit the composer autoloader patch (the one to app/Mage.php) as part of our repo?

[fballiano]

Also, why there's a composer patch when a few lines down there's another support for includes?

[colinmollenhour]

I think the release file could be built with the combination of a composer action like https://github.com/owenvoke/composer-action with https://github.com/actions/create-release.

You're right that the "includes" directory is redundant to the autoloader patch, I think it predates it. Not sure how widely used it is but would say it is ok to remove it in a future release.

[Flyingmana]

Another important thing, we can't release something like this if we don't have a "release builder" that creates the "release.tgz" downloading the module and adding it to "app/code", right? how could we approach that? I could create a script for that.

I've searched github documentation but it seems to me that there's no automatic way to include some build binaries into a newly created release, am I missing something?

have a look at this action here https://github.com/Flyingmana/magento-lts/blob/release-actions/.github/workflows/release.yml#L45

[Flyingmana]

I've also moved zf1 (introducing zf1-future) and pelago_emogrifier to composer.

From what I've tested it work flawlessly

dont mix multiple things in a single PR, please try to do it step by step, and ideally spread it over multiple releases, so we can first gauge the feedback we get, especially if we move required core files out of the repository

[Flyingmana]

Also, why there's a composer patch when a few lines down there's another support for includes?

I cant really look at the diff, because its to many changes for my browser to process it correctly, but the installer adds this patch, because its also used for non-openmage projects.

[fballiano]

Also, why there's a composer patch when a few lines down there's another support for includes?

I cant really look at the diff, because its to many changes for my browser to process it correctly, but the installer adds this patch, because its also used for non-openmage projects.

I know that, my question is that a few lines after that there's an include that mention composer... so I think we should have a native support for the composer autoloader and avoid composer patching the app/Mage.php which seems some kind of workaround from a million years ago :-D

[luigifab]

zf1-future doesn't include some changes made by magento/openmage contributors. I didn't have the full list.

[fballiano]

zf1-future doesn't include some changes made by magento/openmage contributors. I didn't have the full list.

I know and I think it was wrong to do those changes in our repo that way. Anyway I'll move the zf1-future in a separare PR, as requested by @Flyingmana

[fballiano]

I modified this PR to process only Cm_RedisSession and Pelago_Emogrifier, not zend framework. Also I've remove the magento-root-dir from the composer.json

[fballiano]

should I cherry pick #2367 into this PR?

[justinbeaty]

No. If possible, we should use the latest version and raise to ^1.8. (currently testing)

I misunderstood and thought we had to stay with 1.7.x branch. I agree newer is better. Same with PHP 7.0 -- I'm thought we already require 7.2 but looks like it's just not reflected in the README.

---

For the composer.lock mess, the problem is that we should avoid running composer update and updating all packages when we only mean to update a few. So I've again forked this PR, reverted composer.lock to the version in 20.0 and ran just:

composer update --ignore-platform-reqs phpseclib/phpseclib phpseclib/mcrypt_compat pear/net_idna2 colinmollenhour/cache-backend-redis colinmollenhour/magento-redis-session

All seems okay: https://github.com/justinbeaty/magento-lts/actions/runs/2804030203

---

should I cherry pick #2367 into this PR?

It's not necessary. Instead this is a good example of when two PRs which modify omposer.lock will have to be handled in a merge. So when both #2367 and this PR are merged into 20.0, the content-hash will at least conflict. Solution is shown in the following resources:

• https://mikemadison.net/blog/2020/8/18/resolving-composerlock-git-merge-conflicts
• https://stackoverflow.com/questions/46185777/is-content-hash-a-mandatory-part-of-composer-lock

[fballiano]

@justinbeaty I reverted the composer.lock and run your update command :-) let's see

[justinbeaty]

@fballiano If you want, you can update this PR to my branch where I just re-organized the commits on top of latest 20.0. It might help to keep a cleaner git log and in case any one of them needs to be reverted.

https://github.com/justinbeaty/magento-lts/commits/remove_cm_redissession

$ git log --oneline
24a7e61e08 Update composer.lock
4e878d305e Update magento-root-dir and allow plugin cweagans/composer-patches
14332121d2 Move Cm_RedisSession  to composer.json
1085cc7797 Move Cm_Cache_Backend_Redis to composer.json
e7ee80603c Move IDNA2 to composer.json
32aaa5c9a7 Move mcryptcompat to composer.json
879586b98d Move phpseclib to composer.json

Instructions:

# make sure you're on correct branch
git checkout remove_cm_redissession

# make backup just in case
git branch remove_cm_redissession_backup remove_cm_redissession

# add my repo as remote
git remote add justin https://github.com/justinbeaty/openmage.git
git fetch justin

# reset your branch to mine
git reset --hard justin/remove_cm_redissession

# make sure everything looks okay
git log

# force push
git push -f

Let's just wait until the actions are completed: https://github.com/justinbeaty/magento-lts/actions/runs/2804361532

[fballiano]

if we squash as usual it shouldn't be necessary, if I'm not missing anything. I also rebased lately.

[justinbeaty]

It's up to you. Normally I wouldn't want to squash a PR like this because there are many changes that should be separate commits for both git history as well as the ability to revert if needed.

[sreichel]

Should we merge #2367 into 20.0 first?

[fballiano]

IMHO it's better to have 1 PR = 1 commit

@sreichel that's why I asked if I should cherry pick it because I don't know how to merge it in 20.0 otherwise.

[justinbeaty]

IMHO it's better to have 1 PR = 1 commit

I don't always agree, but it's offtopic to this PR anyways.

@sreichel that's why I asked if I should cherry pick it because I don't know how to merge it in 20.0 otherwise.

I really don't think it's necessary. We would avoid a merge conflict if #2367 is merged into 20.0 and this PR is rebased, but in the future I think we are going to have more instances of composer.lock conflicting and need to be able to deal with it. For example we may in the future open several PRs to update each library and we either have to wait until one is merged, rebase the second, etc or deal with the merge conflict. Hopefully in most cases it will just be the content-hash that conflicts and is easy to resolve.

[fballiano]

but to merge it in 20.0 we've to create a pr just for that and it may take days or more to have to merged.

[justinbeaty]

but to merge it in 20.0 we've to create a pr just for that and it may take days or more to have to merged.

This is one of the reasons why I don't think it's necessary to include #2367 in here at all. What if that PR stayed opened for a while, should it block this PR from being merged? I don't see the reason to make one PR dependent on the other in this case because they are not related in any way except that they both modify composer.lock.

[fballiano]

rebased and all green

[fballiano]

question: should we do this kind of change to v19 too since we will need a release builder anyway?

[sreichel]

Yes, please.

Any ideas to build different packages for different php versions? I don't think it's required here, but for Emogriefer (and php <7.3)

[fballiano]

when I rebase it breaks everything so I'll close this one and create a new one on v19

[fballiano]

Any ideas to build different packages for different php versions? I don't think it's required here, but for Emogriefer (and php <7.3)

@sreichel With https://github.com/shivammathur/setup-php#matrix-setup we should be able to do it kinda easily, I'm already porting https://github.com/fballiano/openmage/blob/first_release_builder_workflow/.github/workflows/release.yml but it's difficult to test without having the composer modules actually in the composer.json