WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#25947 closed defect (bug) (fixed)

jshint shouldn't throw errors: wp-admin/js/editor.js

Reported by: dougwollison Owned by: azaozz
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

Pretty much only spacing fixes.

Attachments (5)

25947.diff (36.0 KB) - added by dougwollison 5 months ago.
initial patch; fixed spacing
25947.2.diff (15.8 KB) - added by dougwollison 5 months ago.
Attempt #2; fixed spacing, braces, equals, declared globals (not sure if I used the proper method though).
25947.3.diff (15.6 KB) - added by dougwollison 5 months ago.
Corrected handling of globals. 1 error left; 'switchEditors' is defined but never used
25947.4.diff (15.6 KB) - added by dougwollison 5 months ago.
Revised declaration of globals, restored case of tinyMCE
25947.5.diff (15.6 KB) - added by dougwollison 5 months ago.
Updated to include proper lowercasing of just tinymce

Download all attachments as: .zip

Change History (21)

dougwollison5 months ago

initial patch; fixed spacing

comment:1 dougwollison5 months ago

  • Keywords has-patch added

comment:2 jorbin5 months ago

  • Summary changed from jshint shouldn't throw errors: to jshint shouldn't throw errors: wp-admin/js/editor.js

Please remember to include the filename

comment:3 dougwollison5 months ago

Whoops, forgot while trying to get this submitted from the proper account.

comment:4 nacin5 months ago

Hi dougwollison, thanks for the patch.

It looks like some query.php documentation snuck into this one. (Also: wow, nice work!)

I tested out the patch and the spacing looks great. But it doesn't look like anything was done to fix JSHint errors. :-)

Here's the ones I found when running it:

Running "jshint:core" (jshint) task
Linting src/wp-admin/js/editor.js ...ERROR
[L197:C109] W109: Strings must use singlequote.
    pee = pee.replace( new RegExp( '<p>\\s*(</?(?:' + blocklist + ')(?: [^>]*)?>)\\s*</p>', 'gi' ), "$1" ); 
Linting src/wp-admin/js/editor.js ...ERROR
[L201:C101] W109: Strings must use singlequote.
    pee = pee.replace( new RegExp( '<p>\\s*(</?(?:' + blocklist + ')(?: [^>]*)?>)', 'gi' ), "$1" ); 
Linting src/wp-admin/js/editor.js ...ERROR
[L202:C102] W109: Strings must use singlequote.
    pee = pee.replace( new RegExp( '(</?(?:' + blocklist + ')(?: [^>]*)?>)\\s*</p>', 'gi' ), "$1" ); 
Linting src/wp-admin/js/editor.js ...ERROR
[L204:C98] W109: Strings must use singlequote.
    pee = pee.replace( new RegExp( '(</?(?:' + blocklist + ')[^>]*>)\\s*<br />', 'gi' ), "$1" ); 
Linting src/wp-admin/js/editor.js ...ERROR
[L16:C28] W117: 'tinyMCE' is not defined.
    var t = this, ed = tinyMCE.get( id ), wrap_id, txtarea_el, dom = tinymce.DOM;
Linting src/wp-admin/js/editor.js ...ERROR
[L16:C74] W117: 'tinymce' is not defined.
    var t = this, ed = tinyMCE.get( id ), wrap_id, txtarea_el, dom = tinymce.DOM;
Linting src/wp-admin/js/editor.js ...ERROR
[L33:C17] W117: 'QTags' is not defined.
        QTags.closeAllTags( id );
Linting src/wp-admin/js/editor.js ...ERROR
[L35:C18] W117: 'tinyMCEPreInit' is not defined.
      if ( tinyMCEPreInit.mceInit[ id ] && tinyMCEPreInit.mceInit[ id ].wpautop )
Linting src/wp-admin/js/editor.js ...ERROR
[L35:C50] W117: 'tinyMCEPreInit' is not defined.
      if ( tinyMCEPreInit.mceInit[ id ] && tinyMCEPreInit.mceInit[ id ].wpautop )
Linting src/wp-admin/js/editor.js ...ERROR
[L41:C26] W117: 'tinymce' is not defined.
        ed = new tinymce.Editor( id, tinyMCEPreInit.mceInit[ id ] );
Linting src/wp-admin/js/editor.js ...ERROR
[L41:C46] W117: 'tinyMCEPreInit' is not defined.
        ed = new tinymce.Editor( id, tinyMCEPreInit.mceInit[ id ] );
Linting src/wp-admin/js/editor.js ...ERROR
[L47:C13] W117: 'setUserSetting' is not defined.
      setUserSetting( 'editor', 'tinymce' );
Linting src/wp-admin/js/editor.js ...ERROR
[L58:C22] W117: 'tinyMCEPreInit' is not defined.
        if ( tinyMCEPreInit.mceInit[ id ] && tinyMCEPreInit.mceInit[ id ].wpautop )
Linting src/wp-admin/js/editor.js ...ERROR
[L58:C54] W117: 'tinyMCEPreInit' is not defined.
        if ( tinyMCEPreInit.mceInit[ id ] && tinyMCEPreInit.mceInit[ id ].wpautop )
Linting src/wp-admin/js/editor.js ...ERROR
[L66:C13] W117: 'setUserSetting' is not defined.
      setUserSetting( 'editor', 'html' );
Linting src/wp-admin/js/editor.js ...ERROR
[L1:C18] W098: 'switchEditors' is defined but never used.
var switchEditors = {

"tinyMCE" should be "tinymce", "tinymce" should be declared as a global (as should tinyMCEPreInit, QTags, setUserSetting, and switchEditors). The others (single quotes) should be pretty simple.

comment:5 dougwollison5 months ago

Heh, whoops, thought I reset the repo for that.

As for the jsHint errors, mine wasn't throwing any and I didn't see anything about where the rules file was, took a close look and found it.

New patch coming soon.

comment:6 follow-up: dougwollison5 months ago

Oh, that's why, I'm using the core repo, not the developer repo.

Was that necessary for the inline hook docs? I swear it was suppose to be core.

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

dougwollison5 months ago

Attempt #2; fixed spacing, braces, equals, declared globals (not sure if I used the proper method though).

dougwollison5 months ago

Corrected handling of globals. 1 error left; 'switchEditors' is defined but never used

comment:7 in reply to: ↑ 6 ; follow-up: DrewAPicture5 months ago

Replying to dougwollison:

Oh, that's why, I'm using the core repo, not the developer repo.

Was that necessary for the inline hook docs? I swear it was suppose to be core.

There wasn't a problem because core is a packaged version of develop. Going forward if you're generating patches against trunk you should probably use the develop repo.

comment:8 in reply to: ↑ 7 dougwollison5 months ago

Replying to DrewAPicture:

Replying to dougwollison:

Oh, that's why, I'm using the core repo, not the developer repo.

Was that necessary for the inline hook docs? I swear it was suppose to be core.

There wasn't a problem because core is a packaged version of develop. Going forward if you're generating patches against trunk you should probably use the develop repo.

Understood. I'll keep that in mind and replace my local repo.

comment:9 azaozz5 months ago

Some more notes:

  • Don't think defining the globals for JSHint needs the :false part. This is the default.
  • The globals should probably be comma separated on the same line, following the same standard as var.
  • tinymce is a superset of tinyMCE (tinyMCE exists for back-compat), but tinyMCEPreInit is another global that cannot be changed. Please run some tests with the patched file to confirm all is still working.


dougwollison5 months ago

Revised declaration of globals, restored case of tinyMCE

comment:10 follow-up: dougwollison5 months ago

I'm a little confused though, do I need to change the case on tinyMCE at all or in only some places?

Wait, I see what you mean; I hand changed it on tinyMCEPreInit.

I'll put up another diff with the tinymce change.

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

dougwollison5 months ago

Updated to include proper lowercasing of just tinymce

comment:11 azaozz5 months ago

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

In 26224:

JSHint fixes for editor.js. Props dougwollison, fixes #25947

comment:12 in reply to: ↑ 10 azaozz5 months ago

Replying to dougwollison:

...I hand changed it on tinyMCEPreInit.

Yep, a lot better now :) Thanks.

comment:13 dougwollison5 months ago

Okay, now to just go through my other 10 patches and fix the global declarations on them.

comment:14 follow-up: azaozz5 months ago

No need, whoever commits them can do it.

comment:15 in reply to: ↑ 14 dougwollison5 months ago

Replying to azaozz:

No need, whoever commits them can do it.

Hehe... too late. One or two patches were corrupted anyway.

comment:16 SergeyBiryukov5 months ago

  • Milestone changed from Awaiting Review to 3.8
Note: See TracTickets for help on using tickets.