WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 2 months ago

#38809 reviewing defect (bug)

Better wp namespace in password-strength-meter.js

Reported by: afercia Owned by: davidbaumwald
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch needs-testing needs-testing-info close
Focuses: javascript Cc:

Description

Currently, password-strength-meter.js uses this pattern to pass the wp object as argument:

window.wp = window.wp || {};

var passwordStrength;
(function($){
	wp.passwordStrength = {
...

})(jQuery);

so wp inside the IIFE is, technically, undefined, even if WordPress considers wp a global object. Also the first line is pointless if then window.wp is not passed as argument.

Attachments (2)

38809.diff (401 bytes) - added by lemacarl 3 years ago.
38809.2.patch (584 bytes) - added by davidbaumwald 2 months ago.
Define wp in the IIFE

Download all attachments as: .zip

Change History (15)

#1 @andg
5 years ago

Invoking a typeof on the wp variable in the IIFE actually returns "object", not "undefined", since any variable not otherwise found is attributed to the global window object by default by the browser.

But yeah, passing it as a second argument would remove all uncertainties, and would probably be a better approach overall.

@lemacarl
3 years ago

#3 @lemacarl
3 years ago

  • Keywords has-patch added; needs-patch removed

@afercia Removed window.wp = window.wp || {}; since its pointless

#4 @Hareesh Pillai
5 months ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Future Release to 5.9

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#6 @audrasjb
3 months ago

  • Keywords has-patch needs-testing added; needs-refresh removed

I tested the patch and it still applies cleanly so I'm removing the needs-refresh keyword, but I'm adding the needs-testing keyword as we need to test this thoroughly to make sure it doesn't introduce any regression.

#7 @Boniu91
3 months ago

  • Keywords needs-testing-info added

@audrasjb In this ticket, we should only test regressions related to password strength meter feature, right?

#8 @dkotter
2 months ago

Tested the following scenarios today with this patch and everything worked as expected:

  • Setting a password during the install steps
  • Changing the password of my user account
  • Setting a password when adding a new user
  • Changing the password of that user after they've been added
  • Changing passwords of my account and another user account on a multisite install

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 months ago

#10 @davidbaumwald
2 months ago

  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#11 @audrasjb
2 months ago

As per today's bug scrub:
The patch still applies cleanly against trunk.
Given it was tested successfully, it looks like it safe to go to the next step: get a review from a committer then commit. @davidbaumwald kindly proposed to review the proposed patch.

#12 @davidbaumwald
2 months ago

  • Keywords 2nd-opinion added

After looking through the codebase a bit at similar admin scripts, the window.wp = window.wp || {}; pattern is common. However, the wp variable is aliased inside the IIFE.

Rather than removing the the window.wp = window.wp || {}; line, I think it's probably better to just pass wp to the IIFE. Updated patch incoming.

Adding 2nd-opinion here to see if this is the right approach, or should all other instances of this pattern be revisited.

@davidbaumwald
2 months ago

Define wp in the IIFE

#13 @hellofromTonya
2 months ago

  • Keywords close added; 2nd-opinion removed
  • Milestone changed from 5.9 to Future Release

window.wp is a global,wp is available within the IIFE (as IIFEs can access global variables). See it in action here. I'm not seeing a bug with this implementation, though readability is not the best IMO.

There are multiple types of implementations in the core scripts including the design pattern in the password strength script, injecting window into the IIFE as another param (along with jQuery), and setting the global within the IIFE.

It would be nice to have a consistent design approach across all of these scripts, though outside the scope of this specific ticket. Maybe a new ticket can be opened to explore that type of enhancement.

As the IIFE works with global variables, marking this ticket as a close candidate and moving to Future Release.

Note: See TracTickets for help on using tickets.