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

Remove mcrypt mention in setup documentation #12273

Merged
merged 1 commit into from May 27, 2016

Conversation

@slokhorst
Copy link
Contributor

commented May 25, 2016

mcrypt is no longer mentioned in the 'require' section (removed in commit 692c555), so this is confusing.

Signed-off-by: Sebastiaan Lokhorst sebastiaanlokhorst@gmail.com

mcrypt is no longer mentioned in the 'require' section (removed in commit 692c555), so this is confusing.

Signed-off-by: Sebastiaan Lokhorst <sebastiaanlokhorst@gmail.com>
@codecov-io

This comment has been minimized.

Copy link

commented May 25, 2016

Current coverage is 52.25%

Merging #12273 into master will not change coverage

@@             master     #12273   diff @@
==========================================
  Files           471        471          
  Lines         78650      78650          
  Methods        2105       2105          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          41093      41093          
  Misses        37557      37557          
  Partials          0          0          

Powered by Codecov. Last updated by de030c8...e7e1742

@nijel

This comment has been minimized.

Copy link
Member

commented May 27, 2016

AFAIK phpseclib still works fine with either mcrypt or openssl, so I think the documentation is correct here...

@slokhorst

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2016

As I understand it, mcrypt still is an option but is discouraged in favor of openssl. (see 692c555)
My point is that this line says "the require section says that you can use mcrypt", which is not true because mcrypt is never mentioned anywhere in the docs except for here.

So I think we should either clarify the status of mcrypt, or remove it completely from the docs.

@nijel

This comment has been minimized.

Copy link
Member

commented May 27, 2016

I don't think it's much worse than openssl, but it's definitely better to have mcrypt than nothing. So in the end we should state this in the docs ;-).

@slokhorst

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2016

I'm not sure how bad it is, but mcrypt will be deprecated in PHP 7.1, although it will still be possible to obtain it via PECL after deprecation.

The page that was linked in 692c555 suggests that mcrypt is horrible and should be forgotten ASAP. In that case I don't think it is wise to keep advertising it in the phpMyAdmin docs...

But again; I'm not qualified to make any decision about mcrypt support in phpMyAdmin, I just noticed that the documentation was incoherent.

@nijel

This comment has been minimized.

Copy link
Member

commented May 27, 2016

Indeed it seems that mcrypt is not actively maintained, so it's probably better not to use it for crypto. For example see phpseclib/phpseclib#918

Thanks to pointing this out, I'm merging your changes.

@nijel nijel self-assigned this May 27, 2016
@nijel nijel merged commit eea1bca into phpmyadmin:master May 27, 2016
3 of 4 checks passed
3 of 4 checks passed
Scrutinizer Errored
Details
codecov/patch 100% of diff hit (target 52.25%)
Details
codecov/project 52.25% compared to de030c8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.