JS Client-side file validation is broken (because ajaxPageState is broken?)

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);
      });
    }
  }
};

I believe this is just #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests at work again. Demoting for now.

@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('<div class="messages error file-upload-js-error" aria-live="polite">' + errorMessage.join(' ') + '</div>');

      // 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');
  };

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.

First D8 patch attempt xD.

@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.

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.

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.

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.

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

• Create a test site on SimplyTest.me with Webform, Views, CTools + patch
• Enable Webform, Views, CTools (can I do this using params in above URL?)
• Create a Webform
• Add an input of type file
• View the webform (node/1)
• Select a file for the input
• Check console for error

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?

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.

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...

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

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.

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

I've got a proposed patch over at #2070131: Form & Render Element default properties not properly merged when property is an array of callbacks. 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?

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.

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.

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.

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 :)

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 :)

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).

Any patch for Drupal 7.39?

reroll

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

@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.

reroll

+++ 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?

@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.

@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.

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.

@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.

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.

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.

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

@mbaynton,
You should join my club: #2706483: [policy, no patch] Policy to help less interested Patch move forward.

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

@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.

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!

@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.

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

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

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?

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

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.

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.

Something like this maybe:

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

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

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.

Thanks @nod_, having a look

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.

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

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?

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

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.

#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

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.

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

@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

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

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.

+++ 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.

Reroll for 8.4.x

Fix js style error

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

1. +++ b/core/modules/file/tests/src/FunctionalJavascript/ClientValidationTest.php
   @@ -0,0 +1,93 @@
   +class ClientValidationTest extends JavascriptTestBase {
   

   Yay!

2. +++ b/core/modules/file/tests/src/FunctionalJavascript/ClientValidationTest.php
   @@ -0,0 +1,93 @@
   +    // Verify that the error message is there.
   +    $error_selector = '.file-upload-js-error';
   +    // Verify that there is one and only one error message on the page.
   +    $condition = "jQuery('$error_selector').length == 1";
   +    $this->assertJsCondition($condition, 10000);
   +    $assert->elementExists('css', $error_selector);
   

   This should use \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForElement() I think. This is important because we've had a lot of random fails in JS tests.

3. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -342,10 +342,8 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   -      $element['upload']['#attached']['drupalSettings']['file']['elements']['#' . $element['#id']] = $extension_list;
   +      $element['upload']['#attributes']['data-drupal-validate-extensions'] = $element['#upload_validators']['file_validate_extensions'][0];
   

   This move needs a change record I think. Maybe we should leave the drupalSettings stuff alone in D8 and remove in Drupal 9? Not sure about the BC implications. Will ask the release managers.

It'd be great to get @nod_ and @droplet's view of the changes since #72. They both commented on the fix when it was just fixing the element identifier. Especially the BC part of removing something from drupalSettings.

I discussed the issue with @catch and @xjm. @catch said that he felt drupalSettings is "an alterable data structure." and "with hook_js_settings_alter() it’s fair game to remove specifics for me.". @xjm recommended reaching out to the JS maintainers.

We were trying to get a quick and easy fix, but I'm down with refactoring.

Getting as much stuff as possible out of drupalSettings is a very much desired direction. We did it for #states and it went great. Regarding BC since this thing is not even working I'm pretty sure we're safe. I can't think of why modules would want to access that from the drupalSettings object, if there are issues it's a couple of lines of JS to get the data in the same form as in drupalSettings, not too much hassle.

Since it's refactor time, changed a couple of things:

select element with data attributes, we're moving away from using classes to select elements in JS.
Added a theme function that builds the html of the message
Put back the massaging of extensions on the PHP side, no reason to have more work on the JS side.

Leaving the error displaying for another issue: #77245: Provide a common API for displaying JavaScript messages

Oups forgot to address #98.

Try and clean up some of the failing tests

Merged into new .es6.js

I had a look at why ClientValidationTest is failing, after the changes on #100, but I couldn't find out why.
I have also tested this manually via the UI, and the error messages appear properly...

Perhaps having a look at the other failing tests will help.

Addressing #98.2 in the mean time.

Thanks for taking a stab at the ClientValidationTest failure @Manuel Garcia...it's definitely not obvious.

Here's another attempt to clean up the others.

Good job fixing the other failing tests @mbaynton! Only ImageFieldDisplayTest and ClientValidationTest remaining.

Reapplying my changes of #107 which didnt get into the latest patch.

I had another look at ClientValidationTest, and still no luck:

• Had the test spit out a screenshot just before the failing assertion, and indeed there is no error message displayed (see attached image 201706131833071497342787.9591-after-wait-for-error.jpg).
• Tested this manually using stark, and the error message displays just fine, in its right place and with the proper markup.
• Tested the $script in the js console on the article add page using stark, and it that's not it either...
• Tested the use of the new Drupal.theme function on stark, by manually running the following script on the article add page using stark: jQuery('div.js-form-managed-file').prepend(Drupal.theme('fileValidationError', ['test'], ['jpg'])); - the message is attached properly.

Should you want to generate the screenshot, I added these two lines below the waitForElement assertion on line 88 of ClientValidationTest:

$screen_dir = \Drupal::root() . '/sites/default/files/simpletest/';
$this->createScreenshot($screen_dir . date('YmdHis') . microtime(true) . '-after-wait-for-error.jpg');

So it looks to me like everything should work, and ClientValidationTest should pass... yet it doesn't.

Fixing array syntax

Looks like core/modules/file/file.es6.js underwent a few changes due to #2880007: Auto-fix ESLint errors and warnings

Re-rolled the patch #114

Thanks @Dinesh18 for working on this, but looks like that reroll is not correct.

Re-rolled against 8.4.x. I've included an interdiff against the patch in #117, but it's not very informative.

Thanks @Jo Fitzgerald for the reroll.

Here is some progress on the failing tests, hopefully fixing ImageFieldDisplayTest.

I've spent a bit of time again on ClientValidationTest, and I think I've figured out why the test is not passing.

+++ b/core/modules/file/tests/src/FunctionalJavascript/ClientValidationTest.php
@@ -0,0 +1,94 @@
+    $script = 'var e = jQuery.Event("drop");'
+      . 'e.originalEvent = {dataTransfer: {files: jQuery("input.form-file").get(0).files}};'
+      . 'e.preventDefault = e.stopPropagation = function () {};'
+      . 'jQuery("input.form-file").trigger(e);';
+    $this->getSession()->executeScript($script);

This is where our problem lies I believe. This was based on what QuickEditImageTest does to simulate uploading an image, but after checking, it looks like only quick edit hooks to that event (see image.es6.js line 90), so that drop event does nothing in our case, because we are not using quickedit here.

So I think we need to figure out another way to simulate selecting a file to be uploaded so we can trigger the error message... any ideas?

Reroll the last patch to work on 8.6.x

@Anas_maw thanks, it looks like that patch is missing a few things from the one on #122.

#122 needs to be rerolled against 8.7.x. Still needs work (see #123)

Thisis reroll of #122. That patch is too old and needs rework.

Some info about /core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
During reroll i faced this picture, so this place here needs rework and i just removed bottom part.

<<<<<<< 59f8c0d45dbc23395e9803077bf43cd22755be43:core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
    $this->drupalPostForm(NULL, $edit, $field_name . '_2_upload_button');
    $this->assertSession()->elementNotExists('css', 'input[name="files[' . $field_name . '_2][]"]');
    $this->assertSession()->elementExists('css', 'input[name="files[' . $field_name . '_3][]"]');
=======
    $this->drupalPostAjaxForm(NULL, $edit, $field_name . '_2_upload_button');
    $this->assertNoFieldByXPath('//input[@id="edit-' . strtr($field_name, '_', '-') . '-2-upload"]', 'Upload field for second file not present after it is uploaded.');

    $this->assertTrue($this->cssSelect('input#edit-' . strtr($field_name, '_', '-') . '-3-upload[multiple=multiple]'), 'Third file field presented having multiple attribute');
    $this->assertFieldByName('files[' . $field_name . '_3][]', NULL, 'Third file field has correct name');
    $this->assertTrue($this->cssSelect('input[id=edit-' . strtr($field_name, '_', '-') . '-3-upload][size=22]'), 'Third file field presented having correct size attribute');
    $this->assertTrue($this->cssSelect('input.js-form-file.form-file[id=edit-' . strtr($field_name, '_', '-') . '-3-upload]'), 'Third file field presented having correct css classes');
    $this->assertFieldByXPath('//input[@id="edit-' . strtr($field_name, '_', '-') . '-3-upload" and @data-drupal-validate-extensions=\'["' . $test_image_extension . '"]\']', NULL, 'The data-drupal-validate-extensions attribute is present with the expected value on the upload element.');
>>>>>>> Applying patch from issue 2235977 comment 12159076:core/modules/image/src/Tests/ImageFieldDisplayTest.php

rerolled for latest 8.7.x, also applies 8.8.x, I didn't try 8.9.x.

Rerolled against 9.2.x, checking for any new failures.

Rewrote a bunch of the tests, fixed linting for both PHP and JS. No interdiff because it's basically as big as the patch.

I have this problem in a Drupal 10 project.

The Image field of my Image Media type has these allowed file extensions setup:
gif, jpeg, jpg, png, tif, tiff

However, I cannot upload .tif images because it says its not allowed.
'tif' is also missing from the list underneath the upload file field.
I can however, upload .tiff images (with two 'ff' at the end it works, but with one it doesn't).

UPDATE:
Looking more into this, I am inspecting core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php.

At line 159, the value for $extensions = "gif jpeg jpg png tif tiff".
This is whatever you have entered in your image field allowed extension settings I think.
Then the code gets a list of all supported extensions and removes items from $extensions if they are not in there:

$supported_extensions = $this->imageFactory->getSupportedExtensions();
$extensions = !empty($extensions) ? array_intersect(explode(' ', $extensions), $supported_extensions) : $supported_extensions;

After this, 'tif' is missing form $extensions because apparently it is not inside the $supported_extensions.
So this problem I'm having is apparently 'works as designed'. However I am going to look up if it is correct that .tif is not a supported extension while .tiff is. I find it strange. I'm also gonna try forcing 'tif' extension in the $supported_extensions array just to test uploading and displaying .tif images.

UPDATE:
This comment does not belong here, my apologies. I am not sure where I should post it.
Anyway I found a solution for the problem.
The workaround for when you add .tif to allowed file extensions but drupal still saying that .tif is not allowed when you try to upload a .tif image is via a form alter:
$form['field_media_image']['widget'][0]['#upload_validators']['FileExtension']['extensions'] .= ' tif';