Deprecate retrieving password from $account

Removing code seems fine since it seems not to be supported in D8 any more. I can see how unsetting the pass value could help, but that should be described in the issue more.

+    // Remove the plain text password from the form state.
+    $form_state->unsetValue('pass');

Manual testing for the generated mail tokens is probably a good idea too.

Drupal 6 had the !password token for passwords. There is no official [user:password] token for Drupal 7 or 8. Unfortunately I cant find an issue outlining rationale for removing the password token in D7.

Perhaps this issue should be about adding a password token, and changing the $account->password code to be something that isnt creating dynamic properties.

If it is decided that we dont need a password token, then remove the above code.

For reference, the Registration Password Token project is dedicated to adding this token to Drupal 7 and 8. Both versions rely on this dynamic $user->password value to be set.

Actually this token would need to be made available on user save events to accommodate potential password modifications on the edit form. So this doesnt just affect user registrations

@dpi, I believe the primary reason for removing the password token is that it is bad security practice to display or transmit a user's password in any form. We can argue about what methods of data transfer are and are not safe, but at the end of the day the only 100% guaranteed secure decision is to simply not expose it, and that's the decision the Drupal team have made.

On a technical level (AIUI) it is no longer possible to extract a user's password for display, as the login system has been moved over to a one-way encryption algorithm - the system itself can't decrypt a stored password. (There's really no need to, even at login - instead it encrypts the entered password using the same algorithm and compares it with the stored one.) So that is a pretty big barrier to making such a token generally available even if you wanted to, which, as noted, is a super bad idea.

The RPT module does a very specific job of making the token available in registration emails, which it's only able to do because the password has not yet been encrypted at that point (the form submission is still available). Adding a password token to the system does not look like an practical or desirable option at this time.

Restoring the former task title and category/status.

If this is a bug it will need a test.

We should be careful when removing this, since some contrib modules seem to rely on this property being set (so a bit of an undocumented API?). See for example https://www.drupal.org/project/logintoboggan/issues/1165126.

It doesn't make sense to me that this issue was filed as a bug report. A quick search for other "dead code" issues in the Core queue shows that they're mostly/all Tasks.

I considered the suggested test:

Possibly add test for D8 to insure that the password is the hashed value after save() if no such test exists.

I even wrote one out, but in the end this is no different from a Functional test where the login form is submitted. That's covered by UserRegistrationTest. If I'm wrong, then tell me. Needing a test for this doesn't make sense to me. The other dead code issues I checked don't require a test for it either. So I'm removing the Needs Tests tag (along with other superfluous tags).

Aside from that, I also wondered why PHPStan didn't pick up on this. This line contains an undeclared class property assignment. I figured that should be setting off a standards error. But it doesn't because Core only lints with PHPStan level 1. Those warnings are activated at level 2. You can cause an error to occur manually with the command vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist -l 2 core/modules/user/src/RegisterForm.php.

Still seems to be solving a problem vs just removing code so probably would still vote for a test to show whats needed

#22 should be considered too

Would you prefer to deprecate it instead of removing it immediately?

I added a test.

think this should have an issue summary about why it's needed, not super clear. Also how are we covering instances in contrib that could be relying on this? See #22

I added an explanation at the beginning of the summary.

how are we covering instances in contrib that could be relying on this?

This is an undocumented property. Is it OK to remove it in a major release or do we need to deprecate it first?

Would assume it would need some kind of deprecation.

Wonder if something similar can be done like in EntityBase for original

I opened a new MR that deprecates the property instead of removing it.

Updated title to match what's going on. Am wondering if this one should be removed in 13 but will let committers decide. Seems like a good deprecation.

Thanks for the discussion above. While contrib modules do seem to be using this, given this is a security improvement I think we should just get rid of it sooner rather than later - so let's deprecate in 11.4 for removal in 12. It is obvious we should not be storing plaintext passwords at all and modules that were relying on this feature will have to find another technique if they really need it.

Crediting everyone on this issue for the discussion. Tagging for a followup to remove the deprecation and the setter from main/12.0.

Committed and pushed 9e6f0a2cd9f to main and ba93099d48a to 11.x. Thanks!

Opened #3571296: Remove plaintext $account->password as followup.