search

magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.54k stars 9.32k forks source link

update used version of phpseclib #394

Closed Flyingmana closed 9 years ago

Flyingmana commented 11 years ago

the used version of phpseclib was not updated since magento 1.5, there are a lot of chanegs since. Used version seems to be 19cc43cc1668e179bc4da1782a47d8514e665ff5 / https://github.com/phpseclib/phpseclib/tree/19cc43cc1668e179bc4da1782a47d8514e665ff5

two things to know about.

  1. they changed license to MIT
  2. a lot of changes since them, using versions supported by composer installer would need a refactor because of changed function names.

but you could also use something like this for the start

{
        "require":{
                "phpseclib/phpseclib": "dev-master#19cc43cc1668e179bc4da1782a47d8514e665ff5"
        }
}
orlangur commented 11 years ago

What would be the value of phpseclib update?

Flyingmana commented 11 years ago

I would suggest of using one of the tagged versions.

"0.2.2" would be the next bugfix release, so i suggest for starting the value "0.2.x" which uses the last stable version of the "0.2" minor release.

If you want to update to the next minor version, i would suggest "0.3.x"

its ok to not using a specific commit hash, as during development and maybe even after release, you want to include bugfix releases.

For your official release there is still the composer.lock file, which specifies the exact version you targeted on release date.

vpelipenko commented 9 years ago

@Flyingmana, returning to the initial question. What value do you see in this update?

orlangur commented 9 years ago

Well, back to the question - at least would be nice to remove it from lib/internal/, so, need to take some version bundled as Composer package.

The only place where phpseclib is used is

lib/internal/Magento/Framework/Filesystem/Io/Sftp.php:require_once 'phpseclib/Net/SFTP.php';

The only place where Magento\Framework\Filesystem\Io\Sftp was used is

app/code/Magento/Paypal/Model/Report/Settlement.php:        $connection = new \Magento\Framework\Filesystem\Io\Sftp();

(currently not published with CE)

So, the best option could be to get rid of this library usage by refactoring or use another one. For example, Omnipay is using Guzzle:

There are no dependencies on official payment gateway PHP packages - we prefer to work with the HTTP API directly. Under the hood, we use the popular and powerful Guzzle library to make HTTP requests.

Flyingmana commented 9 years ago

As magento already uses the library via composer now, this issue would be solved for me.

But removing it complete would also be ok I think, as it has a very limited featureset, which mostly has good native replacements. For example there is also a native php extension for sftp connections http://php.net/manual/de/function.ssh2-sftp.php

orlangur commented 9 years ago

Not really loaded via Composer yet, still present in repository code base: https://github.com/magento/magento2/tree/develop/lib/internal/phpseclib

Flyingmana commented 9 years ago

I see, got confused by https://github.com/magento/magento2/blob/develop/composer.json#L160

vpelipenko commented 9 years ago

@Flyingmana, phpseclib is now upgraded to 0.3.10 version, library was removed from source code and added to composer dependencies.

davidalger commented 9 years ago

This issue appears to have been resolved, so I'm going ahead and closing it. Feel free to reopen if you disagree with this assessment.