There is supposed to be a JS check for file extension. Right now it's broken.

CommentFileSizeAuthor
#5 drupal-JS_file_validation_broken-2179655-5.patch3.13 KBjavisr
#5 interdiff.txt556 bytesjavisr
#4 drupal-JS_file_validation_broken-2179655-4.patch556 bytesjavisr
core-js-file-validation.patch3.12 KBnod_
#11 core-js-file-validation-2179655-11.patch910 bytesdroplet
#9 core-js-file-validation-2179655-9.patch2.79 KBnod_

Comments

droplet’s picture

droplet commented 
+++ b/core/modules/file/file.js
@@ -17,28 +17,32 @@
+        .off('.fileValidate', Drupal.file.validateExtension);

it removes all events in .fileValidate namespace (Of course, mostly it's fine)

Other code looks good to me.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Considering the attach behavior kinda owns the .fileValidate namespace I'd say it's fine for the detach to remove all of them. That way if contribs hooks into that they don't need to care about clean-up (since they hardly care about it anyway).

Where my reasoning breaks down is when I left the Drupal.file.validateExtension in there. jQuery is supposed to only remove event handlers using the namespace and the validateExtension function.

So either
+ .off('.fileValidate');
or
+ .off('change.fileValidate');

Not sure I care much, what's your take on it?

nod_’s picture

nod_
Primary language French
Location Lille
commented

Let's go with: + .off('change.fileValidate');

javisr’s picture

javisr commented
StatusFileSize
new drupal-JS_file_validation_broken-2179655-4.patch556 bytes

Made the change suggested in #3.

javisr’s picture

javisr commented
StatusFileSize
new interdiff.txt556 bytes
new drupal-JS_file_validation_broken-2179655-5.patch3.13 KB

Ops, now these are the valid ones

The last submitted patch, 4: drupal-JS_file_validation_broken-2179655-4.patch, failed testing.

The last submitted patch, 4: drupal-JS_file_validation_broken-2179655-4.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community
nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-file-validation-2179655-9.patch2.79 KB

This is a reroll because the indentation standard changed for JavaScript files. No need for commit credit because of this reroll.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

droplet’s picture

droplet commented
Status: Fixed » Needs review
StatusFileSize
new core-js-file-validation-2179655-11.patch910 bytes

@nod_ rerolled Patch #0 and apply the Comment #5 interdiff changes. BUT comment #5 interdiff.txt do not reflect the facts :S

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Right that's an oversight, the patch fixes a JSHint error as well (unused variable).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed f6e6e52 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

lmeurs’s picture

lmeurs commented

Client side file validation still seems broken due to Drupal's unique ID's that are being used for selectors, see #2235977: JS Client-side file validation is broken (because ajaxPageState is broken?).