Do not blow away non-MD5 password hashes in user_update_7000()

This sounds like a really good idea. I suggest we restrict the conversion to hashes matching [0-9a-f]{15}. There could be anything in there.

simple string inspection is much faster than preg match.

re #6 - Dave Reid's last patch cross-posted with mine and uses the same approach (plus he has tests).

Note that using just the string values instead of preg_match() was > 5.x faster in a simple test locally (testing 4 different strings 100000 times each).

Not sure why Dave Reid's tests cause explosions. Here's a version without the test.

#5: 1205138-user-update-7000-non-md5.patch queued for re-testing.

The patch in #8 looks good, it's very simple and it works. It worries me that the test in #5 fails, but it seems like that might be unrelated to the patch and just a problem in the test or the testing framework, since it works.

The code comment doesn't seem to match what the code is doing there... we're not checking for md5(), we're checking for some weird $ mumbo jumbo.

This mumbo jumbo is *also* not explained, neither in this issue nor in this patch. What are we doing this for? Why are we checking that stuff? Is just checking for $ enough?

Also in #4 I asked that we check explicitly for something that looks like a md5. There can be *anything* in there.

simple string inspection is much faster than preg match.

This is a purely useless optimization. The preg_match() here is already *way slower* then the db_query(), and all those are orders of magnitude slower then the hashing/stretching we do after that. There is absolutely no point in optimizing here.

Ok, well if we don't mind using preg_match(), I think this is the most specific test we can make.

[updated the summary]

Update the summary.

Ok great, that looks fine.

Talked with chx and he agreed we should add tests for this. Just a quickie 2-liner that adds a PHPass hash row to the D6 users table and an assertion that it's retained should be enough, I think.

Hmm, well a minor conumdrum is that we cannot actually add a D7 hash, since the D6 {users} table field is not long enough. Here's patch plus added assertion with a truncated hash.

Verified that assertion failed w/out patch and passes with patch from #15.

oops - left a git config in place to strip the patch prefix. Kind of silly the test bot totally rejects -p0 patches now.

Subscribe

Took some time to understand this patch, but now it makes 100% sense. Ready to fly, IMO.

+++ b/modules/simpletest/tests/upgrade/upgrade.user.test
@@ -26,6 +26,9 @@ class UserUpgradePathPasswordTokenTestCase extends UpgradePathTestCase {
+    // Check that a non-md5 has was untouched.

Minor typo, should be "Check that non-md5 hash was untouched."

13 days to next Drupal core point release.

@skottler, not sure I agree - I think the existing comment is grammatically correct.

oh, oops - it's the missing "h"

fixed typo.

Committed and pushed to 7.x. Thanks!