WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#20392 closed defect (bug) (fixed)

user-profile.js causing Javascript error in user-new.php

Reported by: pbiron Owned by: azaozz
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3.1
Component: Users Keywords: 2nd-opinion
Focuses: Cc:

Description

Line 53 of user-profile.dev.js is causing a Javascript error in user-new.php.

The problem is a result of the fact that user-new.php does NOT include include a "nickname" field. Hence, Line 40 results in inputs['display_nickname'] being undefined...and as a result Line 53

var val = value.replace(/<\/?[a-z][^>]*>/gi, '');

causes a JS error since value is undefined when id == 'display_nickname'.

I can think of at least 3 possible fixes:

  1. add a "nickname" field to user-new.php
  2. replace Line 40 with something like display_nickname : $('#nickname').val() ? $('#nickname').val () : '',
  3. wrap lines 53-62 of user-profile.dev.js in if (undefined != value) {}

The 3rd solution seems the most general (and best) to me, since it future-proofs user-profile.js for contexts where any of the fields might not be present. However, I'd really like a 2nd opinion on this before submitting a patch.

Attachments (3)

20392.patch (2.7 KB) - added by azaozz 2 years ago.
20392.2.patch (1.8 KB) - added by SergeyBiryukov 2 years ago.
20392.3.patch (2.9 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.4

I guess I would add

if ( !value )
	return;

before line 53 (similar to point 3).

comment:2 pbiron2 years ago

@sergey: that would work as well, and is probably easier to read.

comment:3 azaozz2 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In [20407]:

Don't try to append "Display Name" options when on the Add New User screen (the <select> doesn't exist there), props pbiron, SergeyBiryukov, fixes #20392

comment:4 follow-up: nacin2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We should probably avoid this to begin with: $('#first_name, #last_name, #nickname').blur( if we're not on the page for it. How much in user-profile.js is the same?

azaozz2 years ago

comment:5 in reply to: ↑ 4 azaozz2 years ago

Replying to nacin:

Yes, can check if that <select> exists first. The pass strength JS is in the same file and is used on the Add New User screen.

SergeyBiryukov2 years ago

comment:6 follow-up: SergeyBiryukov2 years ago

return seems a bit more readable than a long if block.

20392.2.patch also includes some whitespace fixes as per the coding standards.

comment:7 in reply to: ↑ 6 ; follow-up: azaozz2 years ago

Replying to SergeyBiryukov:

return seems a bit more readable than a long if block.

Yes but this is in an anonymous wrapper function that does several things. If it's ever extended to include another jQuery chain, we will have to be careful to add it above or below that return depending where it should run. That seems clearer with an if() block imho.

Last edited 2 years ago by azaozz (previous) (diff)

SergeyBiryukov2 years ago

comment:8 in reply to: ↑ 7 SergeyBiryukov2 years ago

Replying to azaozz:

If it's ever extended to include another jQuery chain, we will have to be careful to add it above or below that return depending where it should run.

I see, thanks for the explanation. 20392.3.patch builds on 20392.patch with just some minor whitespace fixes, while we're at it.

comment:9 azaozz2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [20431]:

Add New User screen: don't bind to blur event on the user_name fields when the <select> doesn't exist, props SergeyBiryukov, fixes #20392

Note: See TracTickets for help on using tickets.