Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
user_password
generates a random alphanumeric password.
The many contrib modules using it would be able to use this more easily if it were a service.
See http://grep.xnddx.ru/search?text=user_password&
Proposed resolution
Create a password_generator service that generates a password using the same algorithm as user_password currently does.
Remaining tasks
User interface changes
API changes
user_password()
is deprecated.
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#27 | 3153085-27-interdiff.txt | 19.36 KB | kim.pepper |
#27 | 3153085-27.patch | 11.1 KB | kim.pepper |
Comments
Comment #2
dpiSounded like it existed already.
Comment #3
kim.pepperThis doesn't seem big enough for it's own component. Maybe just in Utility?
Comment #4
kim.pepperMoved to
Utility\Random::password()
Comment #6
kim.pepperRemoves usages.
Comment #7
kim.pepper❓Looking at the Random class, it only contains instance methods, so adding a static method to it seems a bit wrong. Happy to hear opinions from others?
Comment #8
Krzysztof DomańskiMost of the Utility classes declare static methods. I think it can be static here.
#2564431: Convert the utility functions in Random to statics.
#2671188: Ultility Class Random has many bugs
Comment #9
Krzysztof DomańskiTheRandom
class does not declare static methods like the otherUtility
classes. This is intentional. It is not recommended to use static methods for generating random data. See https://stackoverflow.com/questions/41087939/is-it-good-practice-to-use-static-methods-for-generating-random-numbersComment #10
kim.pepper@Krzysztof Domański you linked to a SO post about C#. There is no state that we need in the static method to worry about thread safety. My question was more about the design of having static methods on that utility. I'm leaning towards a RandomUtils or Password class.
Comment #11
vijaycs85patch in #6 looks goosd to me.
I am not sure either. considering how Random is used, I wonder why the other methods are not static. There isn't any dependency or attributes (other than string and name). In any case, static seems fair for
password()
.Comment #12
vijaycs85Comment #13
DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commentedI'm going to plus one the #6 patch. Having password as a static function is the right way if you ask me.
I don't see any reason why we wouldn't want all of these random methods to be static. Perhaps we could transition more of these functions to static as well as random utilities lend themselves to static methods.
Comment #14
catchThe reason that the other methods on the class aren't static is because they all track the result of previous method calls, to ensure that duplicate strings aren't created in the same request. This is because the methods are mainly used for generating test data.
So... the method can be static, but I'm not sure this is the right place for it to live (but also not sure if there's a better place for it either).
Comment #15
kim.pepperHow about a new Password class in the same Utility namespace?
Comment #16
longwaveI don't see what difference it makes mixing static and non-static methods in the same class, and Random feels like the right place to add this to me.
If we are going to go down the route of #1158720: [policy, no patch] Add parameter type hinting to function declaration coding standards sooner or later, is this worth doing here while we are adding a new method? e.g.
Comment #17
kim.pepperAdds typehinting as per #16
Comment #18
longwaveLooks good to me, back to RTBC. We haven't quite addressed @catch's comments from #14 but I am not sure what a better alternative looks like.
Comment #19
DerekCresswell CreditAttribution: DerekCresswell as a volunteer and at Acro Commerce commentedI couldn't think of a better spot for a 'random' password than the random class. A full class for just a password still seems overkill. If I was looking for a way to generate a random password I would think to type 'random password' not 'password make password' (or whatever it may be).
It would be trivial to add a 'unique' parameter to make sure the password is always unique. Though I don't feel this is to necessary and being static is fine.
Comment #20
alexpottSo this is one of those funny things but this change represents a performance loss on pages that need this method. Because we have one more class to autoload - while user.module is always loaded. OTOH it's one less global function for every other page. So maybe a net gain.
I think #14 is an important point. Mixing this with the test random system is a bit confusing and breaks the expectation that something generated form this class is guaranteed to be unique for that request.
Also one of the common tasks of password policy modules is to require longer or more varied. That means that being able to swap or override this might be useful. In fact it would be good to see what modules like https://www.drupal.org/project/password_policy are doing with respect to password generation and see how making this change could make things easier.
Comment #21
kim.pepperWould you go with a separate class with a static method, e.g. Password::generate()? If we are talking swapping or overriding do we need a service?
I don't think Password Policy does any password generation? Had a look and couldn't see anything.
It does validation and constraints with a module/plugin for each different one:
Comment #22
alexpott@kim.pepper well maybe that's because they can't - there's no way that user_password() can generate a valid password if you have that module installed.
Comment #23
kim.pepperOK. Here's a naive implementation for discussion that takes some options for which character sets to use.
Comment #24
alexpottI think we should refactor the existing function to a service and not change the functionality. This will allow modules like the password policy module to override it. I then think we might consider improving the core implementation - but we shouldn't do that in the same issue.
Comment #25
kim.pepperWe could also go down the path of specifying the number of digits and symbols in the password.
Comment #26
kim.pepperThe challenge here is the method signature becomes the contract. Overriding services might want to change the options per invocation (e.g. exposing a form with options for symbols, digits, length etc) and not with fixed service configuration.
Comment #27
kim.pepperOK. Discussed this with @larowlan in slack, and think a simple service with a default implementation that matches the current behaviour should be ok.
Comment #28
kim.pepperComment #29
longwaveLooks good now, the Drupal\Core\Password namespace is ideal.
Comment #31
catchCommitted b7d7a1d and pushed to 9.1.x. Thanks!