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

Skip the "crypt-blowfish without salt" test for HHVM #12668

Closed
wants to merge 1 commit into from

Conversation

photodude
Copy link
Contributor

Pull Request for Issue crypt-blowfish without salt fails on HHVM due to facebook/hhvm#7420 since our default salt code for crypt-blowfish is resulting in crypt() falling back to DES.

A full fix is in #12428 but requires complicated sufficient regression tests to ensure a password generated pre-patch should still validate correctly post-patch. Since passwords for regression tests would need to be generated by a version of Joomla prior to 3.2.1 when hashPassword() and verifyPassword() were implemented. this gets a bit complicated.

Note: this function is completely removed in 4.0

Summary of Changes

This function is deprecated and no longer used in core, and since this portion of the test has a configuration issue on HHVM it is skipped. Rather that skipping the whole test, we skip only the singular test that is an issue due to the configuration of our "default salt".

Testing Instructions

HHVM will no longer report the failure of this deprecated function that is no longer used in core

Documentation Changes Required

none, although it has been suggested to mark getCryptedPassword() and getSalt() as "as potentially dangerous and only included for backwards compatibility, since 3.2.2 use hashPassword() and verifyPassword() and look forward to 4.0, where getCryptedPassword() and getSalt() functions have been removed."

This portion of the test has a configuration issue on HHVM and is skipped
@photodude
Copy link
Contributor Author

From @mbabker 's comments

Not acceptable. Whether an API is used in core or not, deprecated or not, it should still function as expected until the day it's removed. Likewise, if the class has tests, those should continue to function to validate the behavior.

as such this alternative PR is closed. see #12428 for a solution to this issue

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.

2 participants