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

Composer integration #2171

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Conversation

xiris
Copy link

@xiris xiris commented Apr 15, 2017

This PR aims to move some external libraries from the folder oc-includes to composer.
I added required php-extensions to composer's manifest file and remove some dead code as well.

There is an issue opened #1874 where some users started to discuss the topic.

Almost all external libraries were moved to be managed by composer:

  • ezyang/htmlpurifier
  • smmoosavi/php-gettext
  • pclzip/pclzip
  • phpmailer/phpmailer
  • phpseclib/phpseclib
  • servicescout/akismet

Only one library was excluded from the project because of the native support since PHP >= 5.2:

  • json

Tests

  • PHP 5.6
  • PHP 7
  • Docker (enabling/disabling extensions)

Todo

  • Move Bcrypt to a separate repository or implement other package
  • Move AjaxUploader to a separate repository or do another implementation
  • Remove recaptchalib (Recaptcha v1 on OSClass) and use composer version
  • Use Google/Recaptcha (Recaptcha v2 on OSClass)

Updates (20/04)

  • Move openssl-cryptor to composer

A bit more

The move of oc-includes/osclass will be provided in another PR, as will be huge and require tons of changes. I extracted and added into another repo. I'm doing tons of tests here https://github.com/xiris/osclass-platform/tree/move_osclass_core

@conejoninja
Copy link
Member

Thanks, great work, and definitely the path to follow. I can't remember now if we made any changes to bcrypt and/or recaptcha. I know we did some changes to some libraries we use (like openssl-cryptor).

JSON is included because json_encode/decode could be disabled in the configuration, not sure how frequent it's to be disabled, but we should start thinking about dropping some (most) of the compatibility and works toward 5.6/7.x. Maybe add more requirements at installation.

About recaptcha, we're using the official version from google, why using a third party implementation?

@xiris
Copy link
Author

xiris commented Apr 19, 2017

Hey, thanks, appreciate that!

About the ReCaptcha you are right! I didn't think about it. I will update to use the official repository from google: https://github.com/google/recaptcha

I was researching about the BCrypt and I found this old Gist: https://gist.github.com/hasantayyar/1306679

I compared the files and they are identical! I think the best solution is to create a repository and maintain the class. If you don't want to support it, I found a project with ~4k downloads https://packagist.org/packages/eddieantonio/bcrypt, We could use it as well.

About the compatibility, I think we should try to move fast to work toward PHP >= 5.6.
As a reference, we can check Jordi Boggiano stats about the versions: https://seld.be/notes/php-versions-stats-2016-2-edition

@xiris
Copy link
Author

xiris commented Apr 20, 2017

@conejoninja I finished moving all the external libraries to Composer.
For the "oc-includes/osclass" I think is better to do it in another PR, because will be a huge move. What do you think? I just excluded from the to-do list in this PR.

The AjaxUploader I think is a custom implementation you guys did, right? I created a repository, but I can transfer the ownership to OSClass org and update the PR.

@xiris
Copy link
Author

xiris commented Apr 20, 2017

I was thinking, using the composer we might consider to remove the validation of the extensions from the installation, what do you think?

  • PHP version >= 5.6.x
  • MySQLi extension for PHP
  • GD extension for PHP

We have this on composer.json

@conejoninja
Copy link
Member

Thanks for your job, it's really great.

We are using a pretty old version of AjaxUploader, before it went commercial/paid and free again. As far as I remember it's not a custom, but it had changed too much from were we picked it that's not easy to replace with a new version. What was custom/modified is the PHP code to handle the files upload server-side, but that shouldn't be in the library.

About the requirements during install, we'd left them there for the moment, as most users don't know what composer is and upload their files via FTP, we'll continue to offer a "complete" package with all the required files.

@xiris
Copy link
Author

xiris commented Apr 20, 2017

Hum, got it!
Maybe for the people who downloads from the website, we should give a package with the vendor folder included.

An idea could be to update the dependencies during the installation (the osclass web installer).

@xiris
Copy link
Author

xiris commented Apr 20, 2017

Hey, I saw that someone added a lib for encrypt/decrypt using openssl.
I'm thinking to replace for the following one defuse/php-encryption, as it is a really complete library with tons of use cases.

But there is also a simple one (and good as well), that I believe it works in our case: g4/crypto

@hectorespert
Copy link

@conejoninja @xiris Any news about it?

@xiris
Copy link
Author

xiris commented Jun 23, 2017

@BLACKLEG I finished the integration, just waiting for the merge from the core team.
I will just double check about the openssl-cryptor to see if I already did.

@hectorespert
Copy link

I created a pull request in the composer-installers project to support osclass plugins and themes.

hectorespert added a commit to hectorespert/Osclass that referenced this pull request Oct 22, 2017
Added phpunit dependencies with composer.
composer.json based in osclass#2171
Added some tests
Added travis ci config
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.

3 participants