Problem/Motivation

JavaScript validations applied to file uploads, such as allowed extension restrictions, do not work. The high-level issue is that the validation criteria are represented in a JavaScript object, drupalSettings.file.elements, but it references incorrect element ids for the file inputs.

I thought this was perhaps related to #2179655: JS file validation broken, but AFAICT that's not the case. The symptom is the same though.

Unfortunately, file.js seems to be correct.

Try uploading an image at node/add/article inside CKEditor. file.js causes JS errors. Specifically on
this.value.length > 0
Because this.value, which in turn is because this == the <div>, not the <input>.
That, in turn, is because of $context.find(selector) in file.js receives selector === '#edit-fid-upload', instead of '#edit-fid-upload--2'. Therefor it listens for events at $context (the <div>) rather than at <input>.
That, again, in turn, is because drupalSettings.file.elements does not contain '#edit-file-upload--2'. So either ajaxPageState is broken again/in yet another way, or the HTML ID being generated for the file upload input element is somehow wrong.

Proposed resolution

Rather than storing validation information for each file field in a JavaScript object that is not tightly associated to the input fields, store the validation information as data- attributes in the file input markup.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this is a basic input validation that is present in Drupal 7, with broken code present in 8.
Issue priority Major because this affects all file upload fields for all users.
Prioritized changes Prioritized because it is a bug fix and an improvement to basic user experience
Disruption Non-disruptive outside of core. If changed by moving to validations against data- attributes, the contents of the JavaScript settings.file.elements would change, but currently the (only) data it contains is the allowed extensions so it's very unlikely any other code would have been interested in using this structure.
Files: 
CommentFileSizeAuthor
#96 2235977-94-96.interdiff.txt698 bytesmbaynton
#96 2235977-96.patch15.42 KBmbaynton
#94 2235977-94.patch15.42 KBmbaynton
#88 2235977-88.patch15.4 KBmbaynton
#77 2235977-75-testonly.patch6.49 KBmbaynton
#75 2235977-75-with-functionaljavascript.patch15.4 KBmbaynton
#75 2235977-75.patch12.36 KBmbaynton
#75 2235977-75-testonly.patch6.54 KBmbaynton
#70 interdiff.txt3.79 KBManuel Garcia
#70 js_client_side_file-2235977-70.patch3.98 KBManuel Garcia
#68 interdiff.txt3.83 KBManuel Garcia
#68 js_client_side_file-2235977-68.patch14.95 KBManuel Garcia
#64 interdiff.txt1.75 KBManuel Garcia
#64 js_client_side_file-2235977-64.patch3.08 KBManuel Garcia
#61 interdiff.txt1.82 KBManuel Garcia
#61 js_client_side_file-2235977-61.patch2.76 KBManuel Garcia
#58 2235977-58-client-side-validation.patch961 bytesdroplet
#47 server-error-with-js-error.png64.02 KBkevin.dutra
#46 2235977-38-46.txt6.35 KBmbaynton
#46 2235977-46.patch13.9 KBmbaynton
#38 2235977-38.patch11.37 KBmbaynton
#34 2537930-34.patch10.73 KBmbaynton
#27 2235977-27.patch10.71 KBmbaynton
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,594 pass(es). View
#27 2235977-25-27.interdiff.txt4.37 KBmbaynton
#25 2235977-25.patch10.31 KBmbaynton
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,566 pass(es). View
#23 2235977-js-file-validation-23-inclusive.patch11.76 KBmbaynton
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,499 pass(es). View
#23 2235977-js-file-validation-23-exclusive.patch9.22 KBmbaynton
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2235977-js-file-validation-23-exclusive.patch. Unable to apply patch. See the log in the details link for more information. View
#19 2235977-js-file-validation-19-exclusive.patch5.34 KBmbaynton
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2235977-js-file-validation-19-exclusive.patch. Unable to apply patch. See the log in the details link for more information. View
#19 2235977-js-file-validation-19-inclusive.patch7.99 KBmbaynton
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,443 pass(es), 1 fail(s), and 0 exception(s). View
#12 _patch-core-fix_broken_js_validators-2235977-12-D7.patch2.26 KBlmeurs
#8 issue-2235977-8-drupal-8-core-file-field-no-incrementing-id.png46.4 KBlmeurs
#6 drupal-js-file-validation-id-2235977-5.patch802 bytesswim
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,040 pass(es), 2 fail(s), and 0 exception(s). View
#4 drupal-js-file-validation-id-2235977-4.patch1.06 KBswim
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-js-file-validation-id-2235977-4.patch. Unable to apply patch. See the log in the details link for more information. View
#4 file_upload_selectors_nooooowai.jpg49.92 KBswim
#4 file_upload_selectors.jpg49.35 KBswim

Comments

lmeurs’s picture

I ran into a similar problem on D7 when working on a custom form with managed file fields.

file_managed_file_process() defines the Drupal.settings.file.elements javascript object. It's keys are selectors like #edit-attachment-upload, referring to the file input field <input type="file" />, it's values are strings containing allowed file extensions.

The managed file field is a compound field (parent) which contains the following fields (children):

  1. a file input field and submit button to upload the file or
  2. a link to the file and a submit button to remove the file when a file has already been uploaded.

There are a few problems.

  1. The selectors are based on ID's which are being incremented on AJAX calls, so they are pretty hard to rely on.
  2. Incrementation of parent and child elements is handled separately.
  3. The selector is the parent's ID combined with the "-upload" suffix, which does not take any incrementation into account.

Example

Say the parent's ID is #edit-attachment, the selector for the child becomes #edit-attachment-upload. On each AJAX call (ie. image upload / removal) the parent's ID is being incremented #edit-attachment--2, the selector wrongly becomes #edit-attachment--2-upload, but the child's ID remains #edit-attachment-upload.

The new faulty selector is added to the Drupal.settings.file.elements settings object, which means the old selector still exists in the object: Drupal.behaviors.fileValidateAutoAttach() iterates through all selectors and still will find all elements using old selectors.

Until... a too large file file is being uploaded and validated server side, then the ID of the child file input field is being incremented ie. to #edit-attachment-upload--2 and this field can no longer be addressed by any selector.

Effects

In D7 this results in events not being bound after an invalid file upload. The File module only binds Drupal.file.validateExtension(), so no real harm done. But the auto upload function I am creating (like the one in D8) really relies on working selectors to attach events.

Considerations

Should we still be using ID's that are being incremented or can we start using class names or data attributes (even better for pure javascript purposes)?

Workaround

My temporary workaround is a custom implementation of Drupal.behaviors.fileValidateAutoAttach which converts faulty selectors like #edit-attachment--2-upload into [id^="edit-attachment-upload"], this selector works independently of the file input field's incremented ID. I also applied jQuery.once() to prevent possible duplicate events.

This custom implementation remains in a module of my own (though JS, I used PHP for syntax highlighting):

/**
 * Custom implementation of Drupal.behaviors.fileValidateAutoAttach:
 * - To convert faulty selectors
 * - Add jQuery.once() to prevent duplicate events.
 *
 * @file modules/file/file.inc
 * @link https://www.drupal.org/node/2235977
 */
Drupal.behaviors.fileValidateAutoAttach = {
  attach: function (context, settings) {
    if (settings.file && settings.file.elements) {
      $.each(settings.file.elements, function(selector, extensions) {

        // wd: Convert faulty selectors
        // #edit-attachment--2-upload -> [id^="edit-attachment-upload"]
        if (selector[0] == '#') {
          var newSelector = '[id^="' + selector.replace(/^#/, '').replace(/--\d+/, '') + '"]';

          settings.file.elements[newSelector] = extensions;
          delete settings.file.elements[selector];
          selector = newSelector;

          // console.log(newSelector, selector, $(newSelector, context));
        }

        // wd: Use jQuery.once() to prevent possible duplicate events.
        var extensions = settings.file.elements[selector];
        $(selector, context).once('fileValidateAutoAttach').bind('change', {extensions: extensions}, Drupal.file.validateExtension);
      });
    }
  },
  detach: function (context, settings) {
    if (settings.file && settings.file.elements) {
      $.each(settings.file.elements, function(selector) {
        $(selector, context).removeOnce('fileValidateAutoAttach').unbind('change', Drupal.file.validateExtension);
      });
    }
  }
};
tim.plunkett’s picture

lmeurs’s picture

@tim.plunkett: I think you are right, without the drupal_html_id() issue there would not be a problem in our case.

The keys of Drupal.settings.file.elements are selectors, the values are strings containing allowed file extensions. The problem with the selectors is that they are based on changing unique ID's, the problem with the values is that they can only contain a single upload validator.

Could we consider start using HTML data attributes for this? See #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings.

The selectors are only used to A) find a file input field inside a managed file element to bind events to and B) to find allowed file extension for. The selector could be changed from #edit-my-managed-file-field-upload to .form-managed-file [type="file"] and the allowed file extensions could be stored in a data attribute like data-file-validate-extensions="gif jpg jpeg png".

This would solve lots of problems involving unique ID's, but also open doors for other client side file validators like ie. data-file-validate-size="2048". That is as long as no other modules depend on Drupal.settings.file.elements...

To check the file size I wrote a custom validator which we possibly could integrate into the File module. The next snippet is used on a D7 site, still based on selectors and ID's and uses our own Drupal.settings.sitemail.managedFiles object containing the complete ['#upload_validators'] array.

  /**
   * Helper function to validate a file field's input.
   *
   * The File module validates file extensions and outputs an error message in
   * case of an invalid value. After the File module is done, we validate the
   * size of the selected file. uploadValidators are defined in the form built
   * by Drupal's Form API.
   *
   * @see modules/file/file.js
   */
  Drupal.sitemail.validateUpload = function(e) {
    var errorMessage;

    /**
     * Return if the field has no value or the event has been propagated by
     * earlier executed file validators.
     *
     * NB: IE11 also fires the onchange event when setting the field's value
     * programmatically, so check value before proceeding.
     *
     * @link https://www.drupal.org/node/2301527
     */
    if (!this.value || e.isPropagationStopped()) return false;

    // Remove incrementation from element's ID which is added by Drupal on
    // server side validation, ie. when an uploaded file appeared too large.
    var id = this.id.replace(/--\d+/, '');

    // Iterate through this field's upload validators. At the moment we only
    // check file size, the File module has already checked file extension.
    $.each(Drupal.settings.sitemail.managedFiles['#' + id].uploadValidators, $.proxy(function(key, options) {
      // Validate file size.
      if (key == 'file_validate_size') {
        // Test if file size can be read and if it is allowed.
        if (this.files && this.files[0] && this.files[0].size > options[0]) {
          // Define error message.
          errorMessage = [
            Drupal.t('The specified file %name could not be uploaded.',
              {'%name': this.value.replace('C:\\fakepath\\', '')
            }),
            Drupal.t('The file is %filesize exceeding the maximum file size of %maxsize.', {
              '%filesize': Drupal.sitemail.formatSize(this.files[0].size),
              '%maxsize': Drupal.sitemail.formatSize(options[0])
            })
          ];

          // Exit loop.
          return false;
        }
      }
    }, this));

    // If an error message has been defined.
    if (errorMessage) {
      // Insert .messages element.
      $(this).closest('div.form-managed-file').prepend('
' + errorMessage.join(' ') + '
'); // Set empty value. this.value = ''; // Stop propagating event, even to other events for this element. e.stopImmediatePropagation(); } return true; }; /** * Generates a string representation for the given byte count. The example * from Stackoverlow has been altered to mimic Drupal's format_size() * by making decimals optional and returning ie. 'MB' instead of 'MiB'. * * @link http://stackoverflow.com/a/20463021 */ Drupal.sitemail.formatSize = function(a,b) { return (b=Math.log(a)/Math.log(1024)|0,Math.round(a/Math.pow(1024,b)*100)/100)+' '+(b?'KMGTPEZY'[--b]+'B':'Bytes'); };
swim’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
49.35 KB
49.92 KB
1.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-js-file-validation-id-2235977-4.patch. Unable to apply patch. See the log in the details link for more information. View

Hello,

I found this issue while debugging a Webform AJAX problem. Essentially dealing with the same situation as described above. From what I can see @lmeurs is 100% correct. The id used for the upload field is being built incorrectly after an ajax call & thus the element cannot be found, screenshots attached.

Here is a patch for 7.x.

Do we already have tests for this xD? Added tag.

Status: Needs review » Needs work

The last submitted patch, 4: drupal-js-file-validation-id-2235977-4.patch, failed testing.

swim’s picture

Status: Needs work » Needs review
FileSize
802 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,040 pass(es), 2 fail(s), and 0 exception(s). View

First D8 patch attempt xD.

Status: Needs review » Needs work

The last submitted patch, 6: drupal-js-file-validation-id-2235977-5.patch, failed testing.

lmeurs’s picture

@swim: I reviewed your patches, especially the Drupal 7 version. I reckon you did fix the wrongly composed selector by appending the optional count variable to the end of the selector, but the selector of the child element is still based on the ID of the parent element which increments seperately. See my example from #1:

Say the parent's ID is #edit-attachment, the selector for the child becomes #edit-attachment-upload. On each AJAX call (ie. image upload / removal) the parent's ID is being incremented #edit-attachment--2, the selector wrongly becomes #edit-attachment--2-upload, but the child's ID remains #edit-attachment-upload.

The new faulty selector is added to the Drupal.settings.file.elements settings object, which means the old selector still exists in the object: Drupal.behaviors.fileValidateAutoAttach() iterates through all selectors and still will find all elements using old selectors.

Until... a too large file is being uploaded and validated server side, then the ID of the child file input field is being incremented ie. to #edit-attachment-upload--2 and this field can no longer be addressed by any selector.

A quick fix could be a double selector that tries to find the file input element using it's ID directly or using an attribute selector that queries for ID attributes that start with the upload ID followed by two dashes. Ie.:

$selector = '#' . $upload_id . ', [id^="' . $upload_id . '--"]';

I have not tried this, but I implemented something similar client side. As long as the selector is only used for selecting this element and no other modules use it for other purposes, it should work.

But it's an ugly fix, I prefer using a single general selector like .form-managed-file [type="file"] and storing settings as data attributes on the file input elements themselves. If there is a chance of this approach being accepted I'd be more than willing to supply patches.

NB: The OP set the Drupal version of this issue to 8, but I just tried Drupal 8.0-alpha13 and cannot reproduce appended count numbers (see attached screenshot) which might be the reason why your patch for Drupal 8 fails.

swim’s picture

Hey @lmeur,

Thanks for reviewing my patch, it certainly was a clobbering approach xD. I love the idea RE [meta] Use HTML5 data-* attributes instead of #ID selectors in Drupal.settings. This could of course be implemented via the FAPI which would make building form upload elements a breeze.

The selectors are only used to A) find a file input field inside a managed file element to bind events to and B) to find allowed file extension for.

Does the unique ID serve a purpose for error message position? Or is this done via the name attribute? I really hope your suggestion gets some more momentum as this looks like a huge improvement for Drupal client side validation. Please let me know where/ if I can help.

Wim Leers’s picture

This same problem was reported once again over at #2350061: Javascript error when uploading image through the editor image dialog by Jelle_S:

Stumbled upon this while working on #2061377: Allow image style to be selected in Text Editor's image dialog (necessary for structured content).

When an image is uploaded in the image dialog of ckeditor, I get the following js error: Uncaught TypeError: Cannot read property 'length' of undefined.

Traced it back to /core/modules/file/file.js line 106 where this.value is undefined.

The reason being that the event is bound to an id, but when you select an image, new markup is inserted (adding the 'Upload' button), and a wrapper with the same id as the file input is added around the input, causing the error (a div wrapper does not have a value).

Also, now we have two HTML elements on the page with the same div, which of course is not valid.

Berdir’s picture

I've created #2432671: file-managed-file adds a wrapper div with duplicate id, resulting in JS error today and closed again as a duplicate.

For the record, the duplicate ID's are already there without any ajax calls in my case, just install https://github.com/md-systems/file_entity and go to /file/add to verify.

lmeurs’s picture

For D7: Though the real problem lies within incrementing ID's and wrongly generated ID's of child elements, the best solution in this case still is storing settings in data attributes instead of the JS settings array.

Since I am not sure what the status is of implementing data attributes for D7, I created a patch based on the temporary workaround from #1 which:

  1. Corrects the keys (CSS selectors) of the settings.file.elements array.
  2. Makes sure onchange events are only being bound once to the file fields.
xurizaemon’s picture

D7 patch in #12 does not work for me when using Webform + file field, which could be a Webform issue of course ... To reproduce:

I understand that duplicate IDs is what's causing this (I have a div and an input:file which share #edit-submitted-file-upload on simplytest.me) but are there good reasons to not add && this.value before && this.value.length > 0?

diego.paroni’s picture

The error occurs with the core file modules/file/file.module from Drupal 7.36 and later.
No error with the file.module from Drupal 7.35.

mbaynton’s picture

Comment A:
The patch referenced in #2 has been fixed, but this is still an issue in HEAD (id selectors in settings.file.elements don't match the actual ids.) This is a validation that should work, is it appropriate to bump this back to major?

Comment B:
lmeurs' suggestion in #3 is golden, yes please can we start using data- attributes. I just started working on 2533896, which aims to do exactly what lmeurs mentioned: validate for file sizes. A data- attribute was the obvious choice in my mind for that, and I happily added one, but the music stopped when I added my new Drupal.file.validateSize function into file.js and found the selectors weren't matching so it (and Drupal.file.validateExtension) was never getting called.)

Adding the allowed extensions as a data attribute as well doesn't sound too tricky, but let me read up on that meta issue to see if that's really all that would be needed to make this conversion...

mbaynton’s picture

Priority: Normal » Major

Checked w/timplunkett on IRC, since the issue referenced in 2 did not turn out to resolve this, bumping back up to major

mbaynton’s picture

Issue summary: View changes

Adding beta evaluation. Letting Drupal 8 get to release with a little thing like this known to be broken seems silly; I'll see about a patch.

mbaynton’s picture

Patch converting valid extensions to a data- attribute is in progress, but the attribute is squashed in some cases by the bug from 2070131.

mbaynton’s picture

Status: Needs work » Needs review
FileSize
7.99 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,443 pass(es), 1 fail(s), and 0 exception(s). View
5.34 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2235977-js-file-validation-19-exclusive.patch. Unable to apply patch. See the log in the details link for more information. View

I've got a proposed patch over at #2070131: Form element defaults not properly merged when element item is an array. Because this issue is dependent, I'm providing two patches here, one including the 2070131 patch and one that isolates the code changes directly related to this issue.

There is a known limitation: on <input type="file" multiple="multiple"> fields, only one of the files is evaluated for the proper extension. If it passes, all files are uploaded. The server correctly rejects those that have mismatched extensions, of course, and you're back to the pre-patched behavior. If the one tested file fails, no files are uploaded. I spent hours researching this and decided that fixing it would require more extensive changes than are desirable at this stage; my goal is to see at least somewhat unbroken validation in 8.0. The primary complicating factor is that, for multiple-file fields, one has to somehow selectively stop only some of the files the user selected from actually being uploaded, but form serialization for ajax POSTing is provided by a vendor jquery plugin (jquery-form) which serializes selected files indiscriminately into the POST data. Providing more control over the bytes added for a particular file is very worthwhile in general, because besides letting you do boring things like exclude an image completely, it would also allow contrib modules to do other useful things like rescale too-large images on the client side (you can do that nowadays...). Such modules would be hosed ATM, from what I can see. Perhaps working with jquery-form's author to add some extensibility points upstream is the best longer-term option?

Status: Needs review » Needs work

The last submitted patch, 19: 2235977-js-file-validation-19-exclusive.patch, failed testing.

The last submitted patch, 19: 2235977-js-file-validation-19-inclusive.patch, failed testing.

mbaynton’s picture

Related, #2047151: Clicking Upload before choosing a file breaks the file type restriction on the managed file element . If there's server-side validation breakage this won't fix that, but it will at least cover it up from a UX standpoint.

mbaynton’s picture

Status: Needs work » Needs review
FileSize
9.22 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2235977-js-file-validation-23-exclusive.patch. Unable to apply patch. See the log in the details link for more information. View
11.76 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,499 pass(es). View

New patch. Unrelated to the test failure above, this one also takes a different approach to file fields that allow multiple files: it will test all the files selected, but it stops the upload of all files if even one of them has a disallowed extension. This may or may not be a good thing, its less arbitrary but the downside is that the user has to go back and redo selection of all files if one is bad. The ideal would be the selective uploading mentioned in 19.

Also updated old assertions and added some new ones so the tests pass and check for the new data-drupal- attribute.

The last submitted patch, 23: 2235977-js-file-validation-23-exclusive.patch, failed testing.

mbaynton’s picture

FileSize
10.31 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,566 pass(es). View

Well here's a much less disruptive version of this patch that gets file extension js validation working, does it using data- attributes, and does not depend on any other issues.

I'm not sure how kosher it is to use #after_build FAPI callbacks in core. If that's frowned upon, this could be avoided by continuing to have ManagedFile have specific hardcoded knowledge of certain validation types, but file extensions are hardly the only kind of validation one might want to perform (see #2533896: Make a check of file size a baked-in client side validation for example) and it seems more extensible for the code with the data to put the data in place.

Wim Leers’s picture

This is a pure code style review: there are lots of code style violations.

  1. Can you run eslint? I see JS code style violations.
  2. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
    @@ -279,6 +280,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +   * Form API callback. Attaches data- attributes to the file input field
    +   * for JavaScript's benefit.
    

    80 cols.

  3. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
    @@ -279,6 +280,22 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +   * @param array $element The FileWidget's render array
    +   * @param FormStateInterface $form_state
    +   * @return array The updated render array
    

    This is wrong; look at other places in core for examples on how to write doxygen.

  4. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
    @@ -347,6 +347,9 @@ function testWidgetValidation() {
    +      // check field is marked with expected attr for client-side validation
    

    s/check/Check/
    Missing trailing period.

mbaynton’s picture

FileSize
4.37 KB
10.71 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,594 pass(es). View

Thanks for taking the time to review my patch, Wim. I've done my best to fix the issues.

80 cols.

Hmm? It's under 80 cols -- by so much, in fact, that I could fit another word in. Was that what you were looking for?

In general: I've been unable to write patches that are not reviewed at first for style issues only, sometimes two rounds of it. I'm sure part of this is my lack of inherent familiarity with Drupal standards, but still, there's rather a lot of rules and I'll never be perfect. It occurred to me that in other environments I've worked in with this much concern about style, code analyzers have been available to identify the issues. So I discovered Coder, and thought the days of style-only reviews would be behind me. But, there seems to be a disconnect between what Coder identifies and what people actually care about; it's got something to say about most every file I've pointed it at in core. I don't know if the problem is the code or the validator or excessive official standards, but I would like to know how to write patches that are stylistically acceptable, the first time. My reviewers have all been top core contributors, and that's cool that they have time for these issues and all, but I'm sure I'm not the only one who would rather that I do it right the 1st time :)

Berdir’s picture

Don't worry about receiving coding style reviews. Yes, it would be nice to have tools to ensure this, but that's not so easy.

I have over 700 committed patches for D8 but Wim (and others) still finds tons of nitpicks.. there is always stuff you don't see yourself, nothing wrong with that.

And trust me, it's much better than not getting any reviews at all :)

Dave Reid’s picture

Issue tags: +D8Media
OnkelTem’s picture

What about a patch for Drupal 7.latest? File upload seems to be broken even w/o js editors, just using Overlay (if this matters).

annared’s picture

Any patch for Drupal 7.39?

mgifford queued 27: 2235977-27.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2235977-27.patch, failed testing.

mbaynton’s picture

Status: Needs work » Needs review
FileSize
10.73 KB

reroll

mbaynton’s picture

@OnkelTem, @annared, I can't reproduce this in 7.x HEAD

crmn’s picture

@mbaynton: i got the error and was not able to upload files with a d7 installation and jquery_update for seven (=my admin-theme) configured to 1.5. after changing to 1.10 i am able to upload files again even if the error-message in console still remains.

mbaynton’s picture

Issue summary: View changes
mbaynton’s picture

reroll

Status: Needs review » Needs work

The last submitted patch, 38: 2235977-38.patch, failed testing.

mbaynton’s picture

Status: Needs work » Needs review

The last submitted patch, 12: _patch-core-fix_broken_js_validators-2235977-12-D7.patch, failed testing.

Les Lim’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
@@ -236,6 +236,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#after_build' => array(array($this, 'addValidateDataForJs')),

Instead of creating a addValidateDataForJs after_build callback, why not do this in the existing process callback?

mbaynton’s picture

@crmn ah, so in d7 this is dependent on the jquery version in use? In that case I think it is unlikely that the bug in Drupal 7 is related to the cause in Drupal 8. This issue is tagged for D8, perhaps another issue should be opened for those pursuing this in 7.

mbaynton’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

@Les Lim, using an #after_build was a choice to achieve better decoupling between the ManagedFile element and the FileWidget for fields. Apart from the code I removed, ManagedFile does not know about valid file extensions, since that's something you configure as part of a field. ManagedFile may be used in a field context, or not, but has this code sniffing for field config.

Since I'm also looking at adding client side file size validation, I wanted to nip this in the bud now rather than perpetuating it to ManagedFile sniffing for file size as well.

Les Lim’s picture

Status: Needs review » Needs work

Okay, you're right that you wouldn't want to put it on the FileWidget::process() callback. But I think you might want it to go in ManagedFile::processManagedFile(), though.

Apart from the code I removed, ManagedFile does not know about valid file extensions, since that's something you configure as part of a field. ManagedFile may be used in a field context, or not, but has this code sniffing for field config.

By definition, the ManagedFile element takes an ['#upload_validators'] property, which may contain valid file extensions and/or file size limits. So it's not that ManagedFile is sniffing upstream for field config. FileWidget is passing those settings to the 'managed_file' element. I could do the same thing in a custom form without using a field:

$form['upload_thing'] = [
  '#type' => 'managed_file',
  '#upload_validators' => [
    'file_validate_size' = [8192],
    'file_validate_extensions' = ['exe bat dll sh php'],
  ],
  ...
];

Given that, anything you do for validation should belong to ManagedFile, not FileWidget. In this example, FileWidget is never called.

mbaynton’s picture

Status: Needs work » Needs review
FileSize
13.9 KB
6.35 KB

@Les Lim, you're right, given that it's possible to get the server side validations without the use of a field, it would be a mistake to have the client side only work with fields. Thanks for catching that. Here's a new patch as suggested, plus a test demonstrating the data- attribute appears on direct managed_file usages.

kevin.dutra’s picture

Things seem mostly good with #46. The one thing I encountered (and I don't know if it's outside the scope of this issue or not) is that you can end up with both server-side and client-side errors at the same time.

Multiple errors

To reproduce:

  1. Configure a file field with a small maximum upload size.
  2. Upload a large file that has a valid extension.
  3. AJAX returns with server error for max size.
  4. Upload a file with an incorrect extension.
  5. The extension error (JS) render, but the server-side errors (which don't apply to the file you just tried to upload) are still visible.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mbaynton’s picture

This is 2 years old now...would anyone at NOLA like to help me achieve RTBC?

droplet’s picture

oadaeh’s picture

Issue tags: +neworleans2016

@cweldon and I are triaging this and attempting to figure out what needs to be done next.

oadaeh’s picture

@cweldon has confirmed the functionality is broken in 8.1.1, but he thinks it may be working correctly in 8.2.x. We are still verifying that and what the status of 8.1.x is.

cweldon’s picture

Sorry for the delayed reply. So far it seems the behavior of this issue has been modified (see comment #2235977-47: JS Client-side file validation is broken (because ajaxPageState is broken?)). In the 8.2.x branch I was not able to trigger the AJAX error, however, in the 8.1.1 branch I was able to trigger the error using the steps listed in the problem/motivation section of this issue. Even using simplytest.me I was only able to get the error to trigger in 8.1.1 on the initial install. In other words, after creating an article and going back and attempting to do this again it didn’t seem to trigger the same error. Because I am not able to trigger the issue {re} I was not able to use Git Bisect to see what commit may have modified the behavior of the issue.

As for the duplicate error messages it seems to be in file.js when auto upload is enabled. The error message from the client side validation and the error message from the server side validation are both being displayed. If this is the case, we can possibly add logic on the auto upload to run client side validation and not submit the form if an error is detected. As an alternative we can add logic on the client side validation to not run checks if auto upload is enabled. Personally I would vote for the former since it may take a while for large files to be uploaded and the delay would not be an ideal from user experience prospective.

Note: all of my testing was done in Chrome and Firefox on OSX.

Hopefully this information is helpful to someone who knows more about this component. : )

Thank you to @oadaeh @mbaynton and @Les Lim for all of your help while I was looking into this!

Les Lim’s picture

@cweldon, do you think you would be able to git bisect against 8.2.x to find the commit where the behavior was fixed? :) That seems like a backwards use of git bisect, but it would be just as helpful. I'm wary of fixes that occur by happenstance; it's just as likely that only the steps to reproduce have changed, and not the actual bug.

cweldon’s picture

Good idea, I will try using 8.2.x branch and git bisect.

xjm’s picture

Thanks @oadaeh and @cweldon for helping assess this issue at the major triage sprint. (Updating the issue credit to include triagers.)

cweldon’s picture

I did try going back a few months using Git bisect and didn’t find a commit that caused the behavior of this bug to change. @Les Lim did you have an idea of what what date/commit we could mark as the “bad” commit in Git bisect?

droplet’s picture

Title: JS file validation is broken (because ajaxPageState is broken?) » JS Client-side file validation is broken (because ajaxPageState is broken?)
Issue tags: +Needs JS testing
FileSize
961 bytes

Obviously, it's still broken in D8.1+.

It can be fixed as simple as attached patch. But I'm more interesting which source file appended '-upload' to upload fields. I've grepped source code and can't find anything :s

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nod_’s picture

The upload suffix comes from there : http://cgit.drupalcode.org/drupal/tree/core/modules/file/src/Element/Man... change that key and the suffix changes.

This fixes the issue RTBC once there are tests.

Manuel Garcia’s picture

Here is a WIP test for this, although I haven't figured out how to actually try to upload a file yet.
I've added a todo to the test about this, we could probably make use of this from phantomjs http://phantomjs.org/api/webpage/method/upload-file.html, however i'm not sure how to actually make use of that from within a Drupal test.

nod_’s picture

Something like this maybe:

$session = $this->getSession();
      $session->executeScript("page.uploadFile('input[name=image]', '/path/to/some/photo.jpg');");

Status: Needs review » Needs work

The last submitted patch, 61: js_client_side_file-2235977-61.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
1.75 KB

Did a bit of work, please see the interdiff.

I also tried what you suggested @nod_ but its throwing a JS error:
ReferenceError: Can't find variable: page

Here is the full error provided by running this test via simpletest ui if it helps,
Drupal\Tests\file\FunctionalJavascript\ClientValidationTest::testDisallowedExtensionErrorMessage Zumba\GastonJS\Exception\JavascriptError: One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details). ReferenceError: Can't find variable: page ReferenceError: Can't find variable: page at undefined:1 at :1 at :1 /var/www/drupal8/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:119 /var/www/drupal8/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:99 /var/www/drupal8/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserScriptTrait.php:25 /var/www/drupal8/vendor/jcalderonzumba/mink-phantomjs-driver/src/JavascriptTrait.php:18 /var/www/drupal8/vendor/behat/mink/src/Session.php:324 /var/www/drupal8/core/modules/file/tests/src/FunctionalJavascript/ClientValidationTest.php:52

Status: Needs review » Needs work

The last submitted patch, 64: js_client_side_file-2235977-64.patch, failed testing.

nod_’s picture

We should be able to take the work done in #2421427: Improve the UX of Quick Editing single-valued image fields to test upload, see QuickEditImageTest in the patch.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Issue tags: +Dublin2016

Thanks @nod_, having a look

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
14.95 KB
3.83 KB

ok i have made some progress, not entirely sure this is the way to go, please have a look.

I have used the new ImageFieldCreationTrait that is on #2421427: Improve the UX of Quick Editing single-valued image fields, so this patch requires that one, unless we want to create the image field by hand here.

Test bot will definetly fail because of the trait, but test does pass if you first apply #2421427.

Status: Needs review » Needs work

The last submitted patch, 68: js_client_side_file-2235977-68.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
3.79 KB

Apologies, patch #68 came out really badly, please ignore that one.

Status: Needs review » Needs work

The last submitted patch, 70: js_client_side_file-2235977-70.patch, failed testing.

Dave Reid’s picture

Adding '-upload' to the selector doesn't actually work for me. My issue is that the settings end up like this:

Object {#edit-field-thumbnail-0: "png,gif,jpg,jpeg", #edit-field-video-video-widget-local-upload--Xcah7OjIDm8: "mp4"}

The second upload element is loaded in via AJAX on the form, and has an actual ID of #edit-field-video-video-widget-local-upload-upload--j1IsaerZ-gg. So in addition to matching the -upload suffix, we should likely be matching on the data-drupal-selector attribute instead of the actual element ID?

mbaynton’s picture

Makes me wonder how many miles I'd get out of re-rolling #46 (data- attribute based solution) and splicing in the new tests.

Dave Reid’s picture

I've actually implemented a data attribute for this project, and it works quite well. Relying on selectors vs just adding the data to the element seems like a smart call and would avoid the need for drupalSettings.

mbaynton’s picture

#46 seems to easily merge into 8.3.x. Although it just fixes the extension validation, it's got enough changed stuff that I'm wagering it could be considered more than a simple bugfix for 8.2.x.

Three patches here to better isolate what's working now:
- /data/mywork/coredev/2235977-75-testonly.patch - reroll of only my original tests, which were written before client side JS testing was added.
- /data/mywork/coredev/2235977-75.patch - reroll of #46 for 8.3.x
- 2235977-75-with-functionaljavascript.patch - add @Manuel Garcia's core/modules/file/tests/src/FunctionalJavascript/ClientValidationTest.php as well

The last submitted patch, 75: 2235977-75-testonly.patch, failed testing.

mbaynton’s picture

JS functional tests fail with PHP Fatal error: Trait 'Drupal\image\Tests\ImageFieldCreationTrait' as discussed in #68.

Also I can't make patches, here's the testonly one again.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 77: 2235977-75-testonly.patch, failed testing.

peterarnold’s picture

There is no working patch for D.8.2.x ?

The last submitted patch, 77: 2235977-75-testonly.patch, failed testing.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

The last submitted patch, 75: 2235977-75-with-functionaljavascript.patch, failed testing.

mbaynton’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
15.4 KB

@Manuel Garcia's JS functional testing for this was dependent on #2421427: Improve the UX of Quick Editing single-valued image fields, which was committed today. Here's a minor revision to 2235977-75-with-functionaljavascript.patch that just updates the namespace of ImageFieldCreationTrait to match its new/committed home. It's passing now for me locally.

Version to 8.3.x-dev to align with #2421427: Improve the UX of Quick Editing single-valued image fields

mbaynton’s picture

Status: Needs work » Needs review
Manuel Garcia’s picture

Great news @mbaynton, and thanks for working on this.
I'd RTBC it but let's wait for Dave Reid's input perhaps =)

mbaynton’s picture

Thanks to you too, @Manuel Garcia! once JS testing was added this patch definitely needed it, but I don't know that I would have ever taken the time to figure out how to write them.

Agreed, would be great if @Dave Reid took another look.

mbovan’s picture

+++ b/core/modules/file/file.js
@@ -117,10 +103,10 @@
-      var extensionPattern = event.data.extensions.replace(/,\s*/g, '|');
...
+      var extensionPattern = $(event.target).attr('data-drupal-validate-extensions').replace(/,|\s+/g, '|');

@@ -129,7 +115,7 @@
             '%extensions': extensionPattern.replace(/\|/g, ', ')

Re #19: Just ran into this regex file extensions match problem... Thanks for noticing and fixing it! It also helps with properly adding commas inside the error message.

+1 for #88.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mbaynton’s picture

Reroll for 8.4.x

Status: Needs review » Needs work

The last submitted patch, 94: 2235977-94.patch, failed testing.

mbaynton’s picture

Status: Needs work » Needs review
FileSize
15.42 KB
698 bytes

Fix js style error

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @mbaynton for the reroll and for fixing js style error. I think this is good to go.