Password Policy module relies on a custom menu callback, custom javascript, and reads $_POST directly.

Instead, we should explore using Drupal's #ajax API.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aaronbauman created an issue. See original summary.

AaronBauman’s picture

Status: Active » Needs review
FileSize
7.66 KB

My local apache install is getting in the way of testing this.
Let's see what testbot says.

AohRveTPV’s picture

I plan to review/test this within the next few days.

AohRveTPV’s picture

Been reviewing and this looks like a great improvement!

I'm confused by the duplicate Drupal.evaluatePasswordStrength definitions here. Are both actually needed, or just the first?

  /**
   * Overrides the standard password strength check.
   */
  Drupal.behaviors.passwordOverride = {
    attach: function (context, settings) {
      // Set a default for our pw_status.
      var pw_status = {
        strength: 0,
        message: '',
        indicatorText: ''
      };

      // We are overriding the normal evaluatePasswordStrength and instead are
      // just returning the current status.
      Drupal.evaluatePasswordStrength = function (password, translate) {
        return settings.pw_status || pw_status;
      };
    }
  };

  // We are overriding the normal evaluatePasswordStrength and instead are
  // just returning the current status.
  Drupal.evaluatePasswordStrength = function (password, translate) {
    return Drupal.settings.pw_status;
  };
AohRveTPV’s picture

The password changing behavior seems to be different with this patch: Rather than checking after each keypress in the password field, it checks after the password field loses focus.

I think it'd be better to preserve the current behavior if possible with this change, and then if we want to change the checking behavior, we could do that as a separate issue. I'll try to see how we could modify your patch to have the same behavior.

AaronBauman’s picture

I'm confused by the duplicate Drupal.evaluatePasswordStrength definitions here. Are both actually needed, or just the first?

No idea why this is in there twice, I was just trying to change as little as possible.

Re preserving current behavior: should be as simple as adding an event item in the #ajax array.

AohRveTPV’s picture

No idea why this is in there twice, I was just trying to change as little as possible.

Ah, I see now that your patch didn't introduce that duplication.

Re preserving current behavior: should be as simple as adding an event item in the #ajax array.

Thanks. I'm unfamiliar with the AJAX API. I added a 'keyup' event and it works as before. Except after every keypress, you get a "Please wait..." and spinner, and the browser tab indicates a reload. It's distracting (to me at least), and I wonder if this is why the original authors of this branch didn't use the AJAX API to begin with.

I found a way to get rid of the "Please wait..." and spinner, but I'm not sure there exists a fix for the tab indicating a reload after every keypress. I'll look more into it.

AohRveTPV’s picture

The password field also flashes grey upon keyup (disabled?)--distracting.

Sorry if switching to AJAX turns out to have been a bad idea.