Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This is a follow up issue to #1463624-45: Move password.inc into DIC. As suggested by Rob Loach, a default parameter should be added to the constructor of PhpassHashedPassword
. By also using a constant instead of a naked value, we also regain the advantage that we can document it appropriately in a doxygen comment (instead of an unprocessed plain comment in core.services.yml
as it is now).
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#25 | 1850638-25.patch | 2.24 KB | mfb |
|
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedlog2-count-as-default-argument.patch queued for re-testing.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #4
znerol CreditAttribution: znerol commentedReroll.
Comment #5
Jalandhar CreditAttribution: Jalandhar commentedComment #6
Jalandhar CreditAttribution: Jalandhar commentedPlease review the updated patch which is working fine.
Comment #7
znerol CreditAttribution: znerol commentedComment #14
borisson_This patch doesn't apply anymore, and thus needs a reroll.
When rerolling this issue, please don't rename the variable, only the constant and comment move should be done.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedPerhaps it is worth closing this issue in favor of #2165549: Reduce number of password hashing iterations in all tests to improve test performance. And to discuss what place is better (constructor vs settings).
If we leave it open, then tests are needed. Example, why HASH_COUNT = 16? And how to ensure that this value is not obsolete in future?
From @sun's post it should be:
Also have sense epic works in #1845004: Replace custom password hashing library with PHP password_hash() and https://wiki.php.net/rfc/argon2_password_hash (for happy sites with php 7.2)
Comment #16
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedRerolled the patch, please review it:)
Comment #17
borisson_@mohit1604, the patch in #16 looks good. I don't think we should rename the contructor parameter in this issue though, that seems like scope creep
Comment #25
mfbI'd say this should be closed in favor of #1845004: Replace custom password hashing library with PHP password_hash(). But in any case, rerolled! and reverted scope creep
Comment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedBrought this update in #contribute and @andypost also agrees this is probably a duplicate
Closing in favor of #1845004: Replace custom password hashing library with PHP password_hash()