[wp-trac] [WordPress Trac] #49107: Coding Standards: src/js/media/views/settings.js

[WordPress Trac]

#49107: Coding Standards: src/js/media/views/settings.js
--------------------------+-------------------------------------------
 Reporter:  ankitmaru     |       Owner:  SergeyBiryukov
     Type:  defect (bug)  |      Status:  reopened
 Priority:  normal        |   Milestone:  5.4
Component:  Media         |     Version:  5.3.2
 Severity:  normal        |  Resolution:
 Keywords:  has-patch     |     Focuses:  javascript, coding-standards
--------------------------+-------------------------------------------
Changes (by adamsilverstein):

 * status:  closed => reopened
 * resolution:  fixed =>


Comment:

 @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 );
 }
 }}}

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/49107#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform