Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: Javascript winter clean-up

Run jshint on the files with the configuration from the parent issue or use jshint.com with the following options:

/*jshint forin:true, noarg:true, eqeqeq:true, undef:true, curly:true, browser:true, expr:true, latedef:true, newcap:true, trailing:true */
/*global Drupal, jQuery */

Fix any warnings or errors the tool finds.
Check manually that the fixes did not break any functionalities
Create patch and upload for the testbot.

Files: machine-name.js

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

core/misc/machine-name.js: line 58, col 25, 'machine' is already defined.
core/misc/machine-name.js: line 61, col 107, 'machine' used out of scope.
core/misc/machine-name.js: line 107, col 10, Don't make functions within a loop.
droplet’s picture

Status: Active » Needs work
FileSize
7.03 KB

Anyone give a better var naming.

nod_’s picture

I'd be better to have named functions since they are declared on top, that will help debug/profiling.

For the names, I'd go with clickEditHandler and machineNameHandler sounds fine to me. I'm not the greatest at that stuff though.

nod_’s picture

Status: Needs work » Needs review
FileSize
7.03 KB

changed the name and an unclear this

droplet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

#4 fixed:
- unclear this keyword
- better var naming
- named function declaration for debugging.

Tested:
- Apply and do REAL test
- Reviewed code.

JSHint:
(no error)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

attiks’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Needs work

Follow-up issue, this isn't working the same anymore: #1694650: Form API: Multiple machine names broken

nod_’s picture

Priority: Critical » Major

Not critical, no part of core uses 2 machine names on the same page which would show the bug.

Nice catch though. I was sure the "no function within a loop" could get messy. Better be more careful next time.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
2.39 KB

Attached is a patch that fixes the error (tested manually and with Testswarm Module). Patch is from #1694650: Form API: Multiple machine names broken with the changes @nod_ suggested in that issue

droplet’s picture

would it better if it passing the cached vars as options

Jelle_S’s picture

FileSize
2.54 KB

patch with cached variables, tested manually and with Testswarm.

Jelle_S’s picture

FileSize
2.75 KB

Forgot to clear cache before running Testswarm tests. Previous patch contains bug, this patch doesn't (afaik) ;-)

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly and solves the problem.

nod_’s picture

Status: Reviewed & tested by the community » Needs work

Hang on, missing semicolon and e.data all over the place begs for someone to propose using with(). also it'd be good to prefix the variables with $ for jQuery things.

new patch in a few minutes.

nod_’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

I'm still not happy with how things are done in this file, but that's a matter for another issue: #1686174: Refactor machine-name.js.

put e.data in a variable, will minify better. prefixed jQuery things with $, changed the .click() to .bind() since we're giving it custom data (works with .click() as well but is not consistent with what already exists in core JS files). used preventdefault instead of return false and renamed data to eventData.

works for me.

Jelle_S’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.91 KB

Patch works, tested manually and with testswarm.

put e.data in a variable, will minify better.

Can't you reuse the eventData variable here (in stead of creating a new variable var data), or isn't that considered good practice? jsHint doesn't seem to mind. Created a new patch just in case :-)

EDIT:

Use patch from #15, see #17 for more explanation

nod_’s picture

Oh no that's really bad. It will work, but it's exactly like using a global variable. The machine variable bothers me enough.

This is not a good thing, just like reusing your socks indefinitely. Scope your variables as specifically as possible.

Please test,rtbc and commit patch #15 not the #16 one.

Jelle_S’s picture

Status: Needs review » Reviewed & tested by the community

Ok, patch #15 tested with testswarm and manually. Everything seems to work as it should.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Patch #15 applies cleanly and solves the problem.
@nod_ can you upload the patch again so it's the last one, so committers don't commit the wrong one?

droplet’s picture

why use bind() but not on().

nod_’s picture

consistency for now, the patch changing everything is sill in NR. and most likely needs a reroll.

reuping the patch as #19 suggested.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I have no idea what this does, but the right people seem to think it's a good idea. :)

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.