WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

#42850 closed defect (bug) (fixed)

jshint.js uses non-free license

Reported by: csmall2 Owned by: jorbin
Milestone: 4.9.3 Priority: high
Severity: major Version: 4.9
Component: General Keywords: has-patch fixed-major
Focuses: javascript Cc:

Description

In version 4.9 of WordPress a series of linters were added to the editors to make sure that changes made to things such as themes still worked. This is a good idea.

Unfortunately the linter used for Javascript was jshint. This has a single file which uses the "Do not use for Evil" clause. It basically means anything that uses that file is unable to be distributed. In effect, if anyone makes a WordPress site that is "evil" (whatever that means) they're breaking the license.

You can find the file in wp-includes/js/codemirror/jshint.js

The jshint project is aware of the problem and are trying to fix it. https://github.com/jshint/jshint/issues/1234

I'll be making modifications to the Debian package to just not ship jshint which is a shame but brings it back to pre 4.9 behaviour. Ideally I'd like it fixed in WordPress proper.

Given WordPress does care about the licenses (e.g the React changes) you may want to look into this as well.

Attachments (3)

42850.diff (1.0 MB) - added by jorbin 6 months ago.
42850.1.diff (285.6 KB) - added by jorbin 6 months ago.
42850.2.diff (285.6 KB) - added by netweb 6 months ago.

Download all attachments as: .zip

Change History (18)

#1 @voldemortensen
7 months ago

  • Focuses javascript added

After reading that entire github issue, I've pulled out some useful bits.

IBM got permission to re-license their fork and have re-licensed to the MIT license.

JSHint is currently trying to rewrite the portions that need to be changed to be compatible with that.

According to the [JSHint repo owner|https://github.com/jshint/jshint/issues/1234#issuecomment-331321897], may of the contributors aren't familiar with free code issues and need help rewriting those portions. I think this is a significant opportunity for WordPress to contribute to another project, much like has happened with PHPCS.

#2 @voldemortensen
7 months ago

  • Keywords coding-standards added

This ticket was mentioned in Slack in #core-committers by rmccue. View the logs.


7 months ago

#4 @jorbin
7 months ago

  • Milestone changed from Awaiting Review to 4.9.2
  • Priority changed from normal to high
  • Severity changed from normal to major

We should remove jshint both from our development dependencies ( related: #31823 ) and from codemirror while the license contains this clause.

#5 @dd32
6 months ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

This ticket was mentioned in Slack in #core by jorbin. View the logs.


6 months ago

#7 @netweb
6 months ago

  • Component changed from General to Build/Test Tools

@jorbin
6 months ago

@jorbin
6 months ago

#8 follow-up: @jorbin
6 months ago

  • Component changed from Build/Test Tools to General
  • Keywords has-patch added; coding-standards removed

JSHINT is fairly ingrained into CodeMirror for JavaScript Linting. At first, I looked into just removing it and that means that JavaScript wouldn't be error checked at all in the editor. As that is a less than ideal solution, I've gone and included [esprima](http://esprima.org/) which is available under a BSD license and written a small wrapper in order to convert the errors it generates into the style that CodeMirror is looking for.

I haven't removed any of the JSHINT config stuff in case someone wanted to load there own copy, but this won't be something that can be distributed via a plugin.

For 4.9.3, since we can't include new files, I'll combine fakejshint.js and esprima.js into the jshint.js file.

I want to make sure this is included in the beta for 4.9.3, so I'm going to commit it tomorrow barring objection or alternatives.

The switching of our dev tools use of jshint can continue to happen in #31823 as that isn't bundled with WordPress and this should be purely about the distribution of jshint with WordPress.

#9 in reply to: ↑ 8 @SergeyBiryukov
6 months ago

Replying to jorbin:

I want to make sure this is included in the beta for 4.9.3, so I'm going to commit it tomorrow barring objection or alternatives.

None from me.

@netweb
6 months ago

#10 follow-up: @netweb
6 months ago

Patch 42850.2.diff contains a spelling fix s/liscensed/licensed

Testing wise, does what it says on the box, replaces JSHint with Esprima +1

With that said, I'm not familiar with Esprima and I've not been able to trigger an "error" condition, only "warnings".

Looking at this page http://esprima.org/demo/validate.html it states:

"Unlike a typical code linter, this syntax validator does not care about coding styles and formatting."

"If there is a syntax error, the sign ⚠️ will be shown in the left-side gutter. Placing the mouse cursor over that sign will reveal the complete error description."

Should this implementation here or can the Esprima "warnings" be changed to "errors"

Or should that be looked at for 5.0 and go with this as is for 4.9.3?

#11 @jorbin
6 months ago

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

In 42547:

Remove JSHint due to license compatibility

JSHint inherits a license from JSLint that includes the clause "The Software shall be used for Good, not Evil." WordPress's license specifically allows grants the freedom to run the program, for any purpose. Please note, this is not an encouragement of evil. Rather than doing something evil, how about learning to love those around you. Instead of tweeting lies and saying people are "Not Good!", help your neighbor. In the words of Lin Manual Miranda, "Love is love is love is love is love is love is love is love, cannot be killed or swept aside."

This replaces JSHint with esprima, a part of the larger jQuery project, and a custom wrapper for some basic error checking within codemirror.

The existing JSHint configuration is kept in place in case someone wants to use that, but they can only do so for Good.

Fixes #42850
Props netweb for a spelling fix on a comment.

#12 @jorbin
6 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 in reply to: ↑ 10 ; follow-up: @jorbin
6 months ago

Replying to netweb:

Patch 42850.2.diff contains a spelling fix s/liscensed/licensed

"Unlike a typical code linter, this syntax validator does not care about coding styles and formatting."

"If there is a syntax error, the sign ⚠️ will be shown in the left-side gutter. Placing the mouse cursor over that sign will reveal the complete error description."

Should this implementation here or can the Esprima "warnings" be changed to "errors"

Or should that be looked at for 5.0 and go with this as is for 4.9.3?

I'm absolutely open to improvements for 5.0. In my eyes, getting rid of JSHint for 4.9.3 is a case where perfect is the enemy of complete. Esprima isn't going to solve things forever, but license issue with JSHint issues are resolved in a way that will allow it to come back so that style checks can come back.

#14 in reply to: ↑ 13 @netweb
6 months ago

Replying to jorbin:

I'm absolutely open to improvements for 5.0. In my eyes, getting rid of JSHint for 4.9.3 is a case where perfect is the enemy of complete. Esprima isn't going to solve things forever, but license issue with JSHint issues are resolved in a way that will allow it to come back so that style checks can come back.

Sounds good, I'll have a think about what else we can do to iterate for /trunk and 5.0 +1

For starters, I don't think we should keep a file named fakejshint.js.

#15 @jorbin
6 months ago

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

In 42548:

Remove JSHint due to license compatibility

Backports [42547] without adding new files due to how minor updates work.

JSHint inherits a license from JSLint that includes the clause "The Software shall be used for Good, not Evil." WordPress's license specifically allows grants the freedom to run the program, for any purpose. Please note, this is not an encouragement of evil. Rather than doing something evil, how about learning to love those around you. Instead of tweeting lies and saying people are "Not Good!", help your neighbor. In the words of Lin Manual Miranda, "Love is love is love is love is love is love is love is love, cannot be killed or swept aside."

This replaces JSHint with esprima, a part of the larger jQuery project, and a custom wrapper for some basic error checking within codemirror.

The existing JSHint configuration is kept in place in case someone wants to use that, but they can only do so for Good.

Fixes #42850
Props netweb for a spelling fix on a comment.

Note: See TracTickets for help on using tickets.