Refactor hook_install logic to expire passwords for existing users

+1 on this being good. Installed and tested successfully.

Thanks, this looks good to me after a first pass of reviewing.

The query for loading users in cron has grown more complex and I think it could use some inline comments to explain its parts. I will plan to add those shortly and make a couple other tweaks.

I agree with #3 that parts of the cron function should be abstracted into other functions (if that is what was meant). The function is growing too long. In the 7.x-1.x branch, password_policy_cron() grew into like multiple hundreds of lines. :(

Why does password_policy_install() use gmdate() for formatting the install date when the date.formatter service is used elsewhere for formatting the last password reset date? Seems inconsistent--maybe there's a good reason.

I feel this needs tests. Functional tests can invoke cron. I don't know if there is a good way to mock time methods in Drupal 8. In 7.x-1.x, I did so by writing wrapper functions for all time functions. So, for example, if you tried to get the current time, and the expiration code was under test, it would be the phony time set by the test.

I can try to contribute some tests.

I'm proposing a simpler way to solve this, without needing of a state variable. No interdiff because it's a different approach.

This is critical as breaks the module install on sites with tons of existing users.

EDIT: Of course this means that existing users will have to reset their password when the module is enabled.

I'm looking at using this module for a site with 30k users and I ran in to issue in testing as well. The changes in this patch for hook_install() make sense, but the changes to password_policy_cron() do not because it would expire passwords for all users in chunks of 250 over the course of cron runs after install.

Is there any good reason for this operation to happen during cron anyway? Why not just check the expiration condition on login and send the user to reset if the password is found to be expired? For initial install, a simple config variable could be set with the install time to use a fallback when the field is not yet present for a user.

I could provide a patch for this if the maintainers are on board with the change (and there is no clear advantage to doing this with cron).

Attaching an updated version of the patch from #4. I was not able to apply that patch on the latest dev version of the module (and, incidentally, was not able to interdiff it with my patch, either). Changes in this patch are:

1. Added comments to the cron query building (per #7).
2. Removed usages of gmdate -- I couldn't see any reason to use that over Drupal's service (per #8).
3. Fixed install time check in cron function. The condition in #4 is $install_time && $install_time <= $expire_date, but should be $install_time && $install_time >= $expire_date to ensure that the policy only applies if the expire date occurs after the install time.

And here is a version with tests passing, to show the change... had an affect, anyway (: We still need tests of the actual change in functionality here. This test case should probably:

1. Set up without the Password Policy module installed.
2. Add a new user.
3. Install Password Policy module.
4. Confirm new install time state value set.
5. Confirm new user does not have expiration fields set.
6. Add 1 day reset policy.
7. Run cron.
8. Confirm no change to the test user's expiration fields.
9. Change install time state value to a date in the distant past.
10. Run cron.
11. Confirm user password is expired.

Here's an updated patch with tests for this change.

Rerolled last patch

There was en error on previouse patch. Here is updated reroll.

@piggito's patch in #18 is working great for Guardr installs. Thank you!

Patch in #18 works well. Also #2771129: "Field field_last_password_reset is unknown" while importing configuration on site without password_policy activated gets resolved with this patch.

Patch #18 applied cleanly to 3.x-dev.

Updating issue title and summary to better reflect current patch.

Thank you all, this has been reviewed, tested, and merged.

Can we have a new release pls?