Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#21 | core-js-jshint-machine-name-1684804-15.patch | 2.84 KB | nod_ |
#16 | core-js-jshint-machine-name-1684804-16.patch | 2.91 KB | Jelle_S |
#15 | core-js-jshint-machine-name-1684804-15.patch | 2.84 KB | nod_ |
#12 | 1684804-12.patch | 2.75 KB | Jelle_S |
#11 | 1684804-11.patch | 2.54 KB | Jelle_S |
Comments
Comment #1
nod_Comment #2
droplet CreditAttribution: droplet commentedAnyone give a better var naming.
Comment #3
nod_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
andmachineNameHandler
sounds fine to me. I'm not the greatest at that stuff though.Comment #4
nod_changed the name and an unclear
this
Comment #5
droplet CreditAttribution: droplet commentedThanks.
#4 fixed:
- unclear this keyword
- better var naming
- named function declaration for debugging.
Tested:
- Apply and do REAL test
- Reviewed code.
JSHint:
(no error)
Comment #6
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #7
attiks CreditAttribution: attiks commentedFollow-up issue, this isn't working the same anymore: #1694650: Form API: Multiple machine names broken
Comment #8
nod_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.
Comment #9
Jelle_SAttached 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
Comment #10
droplet CreditAttribution: droplet commentedwould it better if it passing the cached vars as options
Comment #11
Jelle_Spatch with cached variables, tested manually and with Testswarm.
Comment #12
Jelle_SForgot to clear cache before running Testswarm tests. Previous patch contains bug, this patch doesn't (afaik) ;-)
Comment #13
attiks CreditAttribution: attiks commentedPatch applies cleanly and solves the problem.
Comment #14
nod_Hang on, missing semicolon and
e.data
all over the place begs for someone to propose usingwith()
. also it'd be good to prefix the variables with $ for jQuery things.new patch in a few minutes.
Comment #15
nod_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 renameddata
toeventData
.works for me.
Comment #16
Jelle_SPatch works, tested manually and with testswarm.
Can't you reuse theeventData
variable here (in stead of creating a new variablevar 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
Comment #17
nod_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.
Comment #18
Jelle_SOk, patch #15 tested with testswarm and manually. Everything seems to work as it should.
Comment #19
attiks CreditAttribution: attiks commentedPatch #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?
Comment #20
droplet CreditAttribution: droplet commentedwhy use bind() but not on().
Comment #21
nod_consistency for now, the patch changing everything is sill in NR. and most likely needs a reroll.
reuping the patch as #19 suggested.
Comment #22
webchickI 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!