WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

#49107 closed defect (bug) (fixed)

Coding Standards: src/js/media/views/settings.js

Reported by: ankitmaru Owned by: SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.3.2
Component: Media Keywords: has-patch dev-feedback
Focuses: javascript, coding-standards Cc:

Description

Coding Standard: Expected a conditional expression and instead saw an assignment.

Attachments (2)

assignement_condition_bug_fix.diff (461 bytes) - added by ankitmaru 5 months ago.
49107.diff (709 bytes) - added by adamsilverstein 5 months ago.

Download all attachments as: .zip

Change History (8)

#1 @SergeyBiryukov
5 months ago

  • Component changed from General to Media
  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#2 @SergeyBiryukov
5 months ago

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

In 47029:

Coding Standards: Move assignment out of condition in js/media/views/settings.js.

Props ankitmaru.
Fixes #49107.

#3 follow-up: @adamsilverstein
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@ankitmaru Thanks for raising this issue, this is indeed a bit of confusing code. I'm curious what linting rule you caught this with? This code actually does an assignment and logic check, so your patch doesn't quite fix the issue.

@SergeyBiryukov: Despite the linting violation and poorly constructed code here, this change actually brakes the functionality of this code as I am reading it. The comment line above:

If the setting has a corresponding user setting, update that as well.

indicates what the code should do

the line userSetting = $setting.data('userSetting') assigns the $setting.data('userSetting') value and returns a truthy value if userSetting is set, the if() logic is checking that. After [r487029] userSetting will be undefined at L114: window.setUserSetting( userSetting, value );.

Instead, to clear this up and fix the linting error, we can move the assignment above the logic check:

userSetting = $setting.data('userSetting');
if ( userSetting ) {
	window.setUserSetting( userSetting, value );
}

#4 @adamsilverstein
5 months ago

  • Keywords dev-feedback added

#5 in reply to: ↑ 3 @SergeyBiryukov
5 months ago

Replying to adamsilverstein:

Instead, to clear this up and fix the linting error, we can move the assignment above the logic check:

userSetting = $setting.data('userSetting');
if ( userSetting ) {
	window.setUserSetting( userSetting, value );
}

That's exactly what [47029] does, unless I'm missing something?

Note that the changeset is different from the originally proposed patch assignement_condition_bug_fix.diff, which would indeed be an unintended logic change.

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

#6 @adamsilverstein
5 months ago

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

That's exactly what [47029] does, unless I'm missing something?

Right! Nice, I was looking at the original patch here, not your commit. Apologies for the confusion!

Note: See TracTickets for help on using tickets.