Hello all,
I noticed that yesterday a patch was committed to trunk to add PBKDF2
support to the OpenSSL extension. I also noted that in the commit
message, the author indicated that he would have rather added it to
the hash extension, but wasn't able to.
I had added a patch back in January for that very feature (adding
PBKDF2 to Hash). https://bugs.php.net/bug.php?id=60813
Personally, I'd MUCH rather see it in hash(), as it's enabled on far
more installs (every one) than openssl, Because of this, I think it
would be far more useful to add it to hash.
Should I create a pull request for this, to implement it in hash
instead of OpenSSL? What do you think?
Thanks
Anthony
Hello all,
I noticed that yesterday a patch was committed to trunk to add PBKDF2
support to the OpenSSL extension. I also noted that in the commit
message, the author indicated that he would have rather added it to
the hash extension, but wasn't able to.
Why wasn't he been able to?
I had added a patch back in January for that very feature (adding
PBKDF2 to Hash). https://bugs.php.net/bug.php?id=60813Personally, I'd MUCH rather see it in
hash(), as it's enabled on far
more installs (every one) than openssl, Because of this, I think it
would be far more useful to add it to hash.Should I create a pull request for this, to implement it in hash
instead of OpenSSL? What do you think?
Doesn't hurt to have it in both of course... So sure.
Derick
Derick,
I noticed that yesterday a patch was committed to trunk to add PBKDF2
support to the OpenSSL extension. I also noted that in the commit
message, the author indicated that he would have rather added it to
the hash extension, but wasn't able to.Why wasn't he been able to?
I haven't spoken to him, but here's part of his original commit
message ( https://github.com/php/php-src/commit/f4847efc5d58b3375fa0f3269158d5e6ab625c21
)
No easy way to put these in the hash extension since we don't really
support optional
parameters to certain algorithms. Implemented in openssl for now since
it has it already
and is pretty stable.
Doesn't hurt to have it in both of course... So sure.
Ok, I'll add it to my list to forward port it to git and submit the PR...
Anthony
hi Anthony,
I noticed that yesterday a patch was committed to trunk to add PBKDF2
support to the OpenSSL extension. I also noted that in the commit
message, the author indicated that he would have rather added it to
the hash extension, but wasn't able to.Why wasn't he been able to?
I haven't spoken to him, but here's part of his original commit
message ( https://github.com/php/php-src/commit/f4847efc5d58b3375fa0f3269158d5e6ab625c21
)No easy way to put these in the hash extension since we don't really
support optional
parameters to certain algorithms. Implemented in openssl for now since
it has it already
and is pretty stable.Doesn't hurt to have it in both of course... So sure.
Ok, I'll add it to my list to forward port it to git and submit the PR...
I am all for having it in hash as well. But yes, it requires more work
that may need additional RFC (changing the API to allow more options
need discussions).
That being said, I am really not in favor of having it yet in 5.4. For
two reasons, first 5.4 is bugs fixes only, no new feature. The second
reason is the whole thing behind this addition. As it looks self
contained, I am not convinced that it is a good choice, or that it is
well tested or designed (api wised) yet. I would prefer to leave it
master for now and see if it is usable and stable for php-next. I have
to ask to revert this addition in the 5.4 branch.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Pierre,
I am all for having it in hash as well. But yes, it requires more work
that may need additional RFC (changing the API to allow more options
need discussions).
What options are needed? The API refactoring that I have done has all
been to static functions, and extracting methods from public ones. I
didn't need to change any public APIs (that I saw). I'm not sure what
"more options" intended to mean (by you, or by Scott), but I don't see
the need, as it's implemented and passes tests without that. If I'm
missing something, please let me know.
I'm not arguing that it doesn't need an RFC, just trying to clarify
the API change. The only API change would be the addition of the
method (at least based on my implementation)... So if it still needs
an RFC, I'm fine with that. I'll whip one up for mine after I get the
PR tested (I have it applied successfully, just want to verify it a
few more times by re-compiling a few times before submitting the PR
and writing the RFC):
https://github.com/ircmaxell/php-src/tree/hash_pbkdf2
That being said, I am really not in favor of having it yet in 5.4. For
two reasons, first 5.4 is bugs fixes only, no new feature. The second
reason is the whole thing behind this addition. As it looks self
contained, I am not convinced that it is a good choice, or that it is
well tested or designed (api wised) yet. I would prefer to leave it
master for now and see if it is usable and stable for php-next. I have
to ask to revert this addition in the 5.4 branch.
No argument there at all. While it would be nice to get it in 5.4, I
can certainly live with 5.5. Especially with the more rapid release
cycles that have been occurring here (instead of 3 to 4 years between
releases).
Thanks!
Anthony
I've submitted a Pull Request for this feature. I'll whip up an RFC
for it tonight and propose it.
https://github.com/php/php-src/pull/105
Thanks,
Anthony
Pierre,
I am all for having it in hash as well. But yes, it requires more work
that may need additional RFC (changing the API to allow more options
need discussions).What options are needed? The API refactoring that I have done has all
been to static functions, and extracting methods from public ones. I
didn't need to change any public APIs (that I saw). I'm not sure what
"more options" intended to mean (by you, or by Scott), but I don't see
the need, as it's implemented and passes tests without that. If I'm
missing something, please let me know.I'm not arguing that it doesn't need an RFC, just trying to clarify
the API change. The only API change would be the addition of the
method (at least based on my implementation)... So if it still needs
an RFC, I'm fine with that. I'll whip one up for mine after I get the
PR tested (I have it applied successfully, just want to verify it a
few more times by re-compiling a few times before submitting the PR
and writing the RFC):
https://github.com/ircmaxell/php-src/tree/hash_pbkdf2That being said, I am really not in favor of having it yet in 5.4. For
two reasons, first 5.4 is bugs fixes only, no new feature. The second
reason is the whole thing behind this addition. As it looks self
contained, I am not convinced that it is a good choice, or that it is
well tested or designed (api wised) yet. I would prefer to leave it
master for now and see if it is usable and stable for php-next. I have
to ask to revert this addition in the 5.4 branch.No argument there at all. While it would be nice to get it in 5.4, I
can certainly live with 5.5. Especially with the more rapid release
cycles that have been occurring here (instead of 3 to 4 years between
releases).Thanks!
Anthony
Hi!
That being said, I am really not in favor of having it yet in 5.4. For
two reasons, first 5.4 is bugs fixes only, no new feature. The second
I disagree. I've always stated small, self-contained features that do
not involve infrastructure changes may be OK, and the RFC explicitly
says so too. If this particular feature is not having consensus, fine,
let's revert it, but in general saying "5.4 is bug fixes ONLY" is not
correct, self-contained function additions, especially ones that have
good usage potential, may be also approved.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
hi Stas,
I disagree. I've always stated small, self-contained features that do
not involve infrastructure changes may be OK, and the RFC explicitly
says so too.
Yes, and trivial or self contained and can be discussed on a case by
case basis. I do not see a discussion nor a proposal here, only a
commit followed by a merge. Even the SSL maintainers were not involved
at all. That's not an acceptable way to deal with such things, sorry.
If this particular feature is not having consensus, fine,
let's revert it, but in general saying "5.4 is bug fixes ONLY" is not
correct, self-contained function additions, especially ones that have
good usage potential, may be also approved.
Approved if proposed, well thought, well implemented and well tested.
With all respects to Scott's work, not the 1st time that it happened.
We already have issues in this exact extension because of bad API
choices. A little momentum is always necessary to get things done
right.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org