Problem/Motivation

In file.module, file_managed_file_save_upload() has been changed to use the new way of setting form errors, but file_save_upload() still uses a lot of drupal_set_message().

Because of this inline form errors are impossible and the upload form element is not highlighted as having an error.
This affects Drupal core as-is and is even more apparent with Inline Form Errors enabled.

In case a field is required, additionally a nothing saying error is shown as well.

This issue has been split off #1493324: Inline form errors for accessibility and UX because: a) the patch is pretty much done otherwise b) we have no test coverage for it c) we might want an API change to fix it.

Summary of the discussion.

Before (with and without IFE
Field not highlighted.

Not helpful extra error message when field is required.

After (without IFE)
Field is highlighted and extra error is not visible anymore.

After (with IFE)

Proposed resolution

We should pass in the $form_state in a new wrapper function around file_save_upload() and use $form_state->setError() or $form_state->setErrorByName() instead of drupal_set_message().

Timplunkett notes that "almost all" calls to file_save_upload are from functions with $form_state. And even the most popular contrib modules do so.
dmsmidt, notes that if it is called outside a scope with $form_state the original backwards compatible file_save_upload() function can be used.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions done
Add automated tests Instructions
Draft a change record for the API changes Instructions
Improve patch documentation or standards (for just lines changed by the patch) Novice Instructions done
Update the patch to incorporate feedback from reviews (include an interdiff) Instructions
Manually test the patch Novice Instructions done
Add steps to reproduce the issue Novice Instructions
Embed before and after screenshots in the issue summary Novice Instructions done
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

Solution

New temporary function (internal/deprecated):

_file_save_upload_from_form($element, FormStateInterface $form_state, $delta = NULL, $replace = FILE_EXISTS_RENAME

Replaces function calls to (in contexts where $form_state is available):

file_save_upload($form_field_name, $validators = array(), $destination = FALSE, $delta = NULL, $replace = FILE_EXISTS_RENAME)

Note: the last function will stay available for BC and for cases that $form_state is not available.

Files: 

Comments

dmsmidt’s picture

Issue summary: View changes
tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +API change
FileSize
11.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,144 pass(es), 26 fail(s), and 0 exception(s). View
tim.plunkett’s picture

Issue tags: +Needs tests

Status: Needs review » Needs work

The last submitted patch, 2: file_save_upload-2482783-2.patch, failed testing.

tim.plunkett’s picture

Oh we might not be able to do this, since it is called from submitForm sometimes and not just validateForm:

Form errors cannot be set after form validation has finished

idebr’s picture

Issue tags: +needs backport to D7

The same error occurs in D7: after a failed file upload the 'for' attribute on the label no longer matches the input file id. A use case for the label is a replacement for the 'upload' button in a somewhat advanced theme. As a result, no new file can be uploaded after an error message.

mgifford’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,448 pass(es), 16 fail(s), and 0 exception(s). View

There were some conflicts in core/modules/file/file.module but it was mostly in the documentation. It was essentially a re-roll.

Status: Needs review » Needs work

The last submitted patch, 7: file_save_upload-2482783-7.patch, failed testing.

The last submitted patch, 7: file_save_upload-2482783-7.patch, failed testing.

xjm’s picture

mgifford’s picture

Re-rolling #7 as it no longer applied.

Status: Needs review » Needs work

The last submitted patch, 11: file_save_upload-2482783-11.patch, failed testing.

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.

The last submitted patch, 11: file_save_upload-2482783-11.patch, failed testing.

mgifford’s picture

Issue tags: +accessibility

I'm tagging this for accessibility as it's required to get inline form errors as a fully fledged part of Core.

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.

mgifford’s picture

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

We're running against the clock on this, so would need tests so we can have something that helps us meet this deadline for 8.3:
https://www.drupal.org/node/2504847#comment-11750733

alexpott’s picture

Also the current patch breaks BC - I think we're better creating a new method which takes the form state and leaving file_save_upload alone - or trying to do something clever and wrapping the method in something that extracts any errors from drupal_set_message().

alexpott’s picture

Also all usages of file_save_upload() are followed by something like:

  /**
   * {@inheritdoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {
    $this->file = file_save_upload('file', $form['file']['#upload_validators'], 'translations://', 0);

    // Ensure we have the file uploaded.
    if (!$this->file) {
      $form_state->setErrorByName('file', $this->t('File to import not found.'));
    }
  }

So any error we set on the element is likely to be overwritten :( I'm not sure of the best approach. Do we really need to do anything here since it is likely an error is going to be set on the form element. Yes there are going to be the messages from file_save_upload() too but that should be okay no?

alexpott’s picture

Here's my idea for wrapping the file_save_upload() method... just posting here for posterity and maybe later use. I think the key thing to answer is #19.

function file_save_upload_from_form(FormStateInterface $form_state, $form_field_name, $validators = array(), $destination = FALSE, $delta = NULL, $replace = FILE_EXISTS_RENAME) {
  $errors_before = drupal_get_messages('error');
  $files = file_save_upload($form_field_name, $validators, $destination, $delta, $replace);
  $new_errors = array_diff(drupal_get_messages('error'), $errors_before);
  if (!empty($new_errors)) {
    // Render the errors into a single message.
    foreach ($new_errors as $error) {
      $form_state->setErrorByName($form_field_name, $error);
    }
    // Empty the old queue and put the errors back as before.
    drupal_get_messages('error', TRUE);
    foreach ($errors_before as $error) {
      drupal_set_message($error, 'error');
    }
  }
  return $files;
}
dmsmidt’s picture

My 2 cents about #19: let's think about how we got to this issue in the first place. That was due to inline form errors. So if we don't do anything with it, those generated messages will never show up inline, the place where people will expect them. And we can't create an anchor link to the error message. Added the related issue: #2687249: Improve inline form errors for file upload fields.

It's a pity we need something like that wrapping logic, but if we document the work around I'm oke with it. Should we then rethink it for D9?

I haven't tested the code in #20, but I think that doesn't work completely in case of multiple errors.

 foreach ($new_errors as $error) {
      $form_state->setErrorByName($form_field_name, $error);
    }

The setErrorByName() method should only be used once per field name, later calls will just be ignored internally.
See:

$errors = $this->getErrors();
if (!isset($errors[$name])) {

This is something wonky about how forms work currently, because we have a render element property '#errors' (plural). While we only accept and work with one error. (But this is something for another issue).

So, I guess the errors should be concatenated into one message.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

I've started working on the wrapper solution.

dmsmidt’s picture

Just an update, still working on this.
The wrapper function seems a good idea, but there are some complications.

I'm not 100% clear about how strict BC has to be, so if someone can enlighten me, please.
For now I assume that we have to keep the input and output of file_save_upload() the same.

The wrapper does covers @tim's concern in #5.

Oh we might not be able to do this, since it is called from submitForm sometimes and not just validateForm: Form errors cannot be set after form validation has finished.

With the wrapper, all calls from submitForm to file_save_upload() will stay in tact. In general I think it is not so smart to call file_save_upload() after validation because then the user can't fix the wrong upload anymore, but that aside, we also don't need to have the error in $form_state any more at that point.

The complication however is in how uploads in form submissions are currently named. After form submission the uploads are in $_FILES. They are keyed by the $parents structure of the form element, but instead of using '][' as glue to implode that structure, the '_' sign is used. So when we fetch the uploaded files we can't be sure anymore about the original $parents structure, since '_' is also valid in field/element names. E.g. ['container', 'my_fieldset', 'field_upload', '0'] becomes 'container_my_fieldset_field_upload_0', instead of 'container][my_fieldset][field_upload][0'. The later is normally used in the Form API and can be reliably be converted back to the $parents structure, where as the first cannot. To be able to set the errors correctly we really need to be able to deduce $parents.

dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Needs review
FileSize
6.12 KB

A well, good that I did that write down.
Here is a patch that does not bother with the strange $_FILES keys.
Logic is backward compatible.
Still needs good testing and tests.

Status: Needs review » Needs work

The last submitted patch, 24: 2482783-24-file_save_upload_form_state-D8.patch, failed testing.

dmsmidt’s picture

Issue summary: View changes
dmsmidt’s picture

Added screenshots and api change description.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

Working on tests

dmsmidt’s picture

Title: file_save_upload() needs $form_state in order to be able to set errors correctly » File upload errors not set and shown correctly.
Assigned: dmsmidt » Unassigned
Priority: Normal » Major
Status: Needs work » Needs review

Just got out of the Usability meeting, and we agreed this is actually a Major issue in core.
As you can see in the 'before' screenshot, currently the errors handling is really broken.

Additionally, fixed the test fails.

Still need test for this API addition. I have a hard time figuring out what is the best way to test this.
Actually because we changed the file_save_upload() call to the new file_save_upload_from_form() in file_managed_file_save_upload() I think other tests still cover if file upload works.
We maybe just have to change some test descriptions and add a test to see if the file upload element really has an/one error in the $form_state.

dmsmidt’s picture

20th’s picture

Sorry for commenting when you have basically already done all the work, but this issue has been on my mind for a few days.

Adding a wrapper around a broken function feels like.. doing things backwards. And since contrib modules won't even use this wrapper, error messages for file uploads will still be broken.

A more direct solution is to do like everywhere else in core - implement new improved functionality and transform old functions into simple wrappers that exist only to preserve API compatibility and can be easily deleted at any moment.

This issue tries to solve two problems:

  1. implement inline errors for file uploads;
  2. preserve backwards compatibility.

To solve the first part, for example, we could create a new testable, mockable, configurable service file_uploader that can:

  • have plugable storage backends and customized destination
  • be extended with plugins to allow custom validation from contrib modules
  • be injected with config and Request instances from container instead of using \Drupal::config(), \Drupal::request() and \Drupal::currentUser()
  • be properly implemented to have separate 'validate' and 'submit' steps
  • throw exceptions in case of errors
  • fix structure of $_FILES keys (as mentioned in #23)

For backwards compatibility it is important to keep only function name and signature, return value and side effects, but not the implementation. So file_save_upload() can be then reimplemented something like this:

function file_save_upload($form_field_name, $validators = array(), $destination = FALSE, $delta = NULL, $replace = FILE_EXISTS_RENAME) {
  $file_uploader = \Drupal::service('file_uploader');
  try {
    $file_uploader->validate($form_field_name, $validators);
    return $file_uploader->save($form_field_name, $destination);
  } catch (FileUploadException $e) {
    foreach ($e->getValidationErrors() as $error) {
      drupal_set_message($error, 'error');
    }
  }
}
dmsmidt’s picture

@20th, better late than never, thanks for thoroughly thinking about this issue.
Of course we want to do this as clean as reasonable possible.
Let me shed a bit of a different light on this, and please let me know what you think about it.

IMHO, the first problem that this issue tries to to solve is not implement inline errors for file uploads anymore.
If you check the new before screenshots, you see that even without IFE, this is pretty broken in D8. So what we want to do is: mark the file upload field (visually/and in thus if $form_state) if it has problems, and get rid of this 'null' message which is a bug.

Furthermore, contrib modules that use the managed_file field will automatically benefit from this. But, modules that call file_save_upload() directly won't indeed.

@alexpott introduced the idea of a wrapper, but I think we actually introduce a new function for a different context.
Just like file_managed_file_save_upload() is added for manged files v.s. normal files, we now add file_save_upload_from_form() for when we still can prevent a form submission, due to an error that need to be fixed.
file_save_upload() is not broken as-is (although not perfect), but it is in core (and probably contrib) being called after form validation. Throwing errors after form validation is not helpful (the user can't fix his form submission), but we sometimes do, and in this case the old function works. These flows need to be addressed later I think.

About the $_FILES, glad you agree this needs work. However if we do so, also the DOM input element names change. If people styled or had some JS based on the #name attribute, their implementation will be broken. In light of BC I didn't dare to change that.

I like the idea to make the whole file handling more OOP, but I won't be able to do that before 1 Feb this year (8.3 deadline). If you can, please, but maybe we should make a new refactoring follow up of that. First of all we need to fix a bug here.

Any feedback on how to best proceed is appreciated.

20th’s picture

Status: Needs review » Needs work

Ok, I see your point. I don't think that I agree about what kind of purpose this new function will serve, but you are right, there is not much time left before 8.3.x.

Adding file_save_upload_from_form() does not create any obstacles for adding a file_upoader service, as I've described it, at a later point, so I'm ok with your solution for now.

That said,

  1. +/**
      * Saves file uploads to a new location.
      *
    + * Note: this function should not be used during form validation, use
    + * file_save_upload_from_form() instead.
    + *
      * The files will be added to the {file_managed} table as temporary files.
    

    Maybe it is better to simply add a @depricated tag to the function documentation instead of this notice.

  2. +        ),
    +      );
    +      $error_message = $this->renderer->renderPlain($render_array);
    +    }
    +    else {
    

    The $this variable is not available here because this function is not a class method.

  3. I guess this still needs tests.
alexpott’s picture

Issue tags: +Needs change record
+++ b/core/modules/file/file.module
@@ -763,7 +836,7 @@ function file_save_upload($form_field_name, $validators = array(), $destination
-        // Unknown error
+      // Unknown error

@@ -1207,7 +1280,6 @@ function file_managed_file_save_upload($element, FormStateInterface $form_state)
-

These are out-of-scope changes.

I still think we need to answer #19. The new approach will change the error messages since the message in example won't ever appear. That might be desirable but then we should also probably remove later calls to setError().

+++ b/core/modules/file/file.module
@@ -1192,7 +1265,7 @@ function file_managed_file_save_upload($element, FormStateInterface $form_state)
       $form_state->setError($element, t('Files in the @name field were unable to be uploaded.', array('@name' => $element['#title'])));

For example, Is there any point to this now?

We definitely will need a change record for this one.

dmsmidt’s picture

Thank you both for that input.

@20th I'm not really sure if we really need to deprecate the old function. People still can use it after form validations (although they shouldn't I think). But I'm not clear about what the requirements are to name something deprecated.

@alexpott, about the question in #19. I think, yes, we need to set an error as soon as possible. Errors will never be overwritten for an element in $form_state. The first is persistent, and the first error should also be the one that needs fixing by the user. I explained that in #21. Or do I miss another question here? So, yes we can remove the setError() you mention.

Could anybody help creating tests or give me an idea which tests to modify? There are so many for file.module.

dmsmidt’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
1.92 KB
6.86 KB

All patch comments have been addressed.

Draft change record created: https://www.drupal.org/node/2844730, please review.

Still tests needed. Anybody, feel free to take this on.

alexpott’s picture

I've begun a test - basically just copying the \Drupal\file\Tests\SaveUploadFormTest. The test still needs to assert that the no messages are set by file_save_upload_form_form() and still needs to assert that all the errors are attached to the form element but the basis for a full test is there. Can someone else please take this up?

Also fixed a few bugs in the other changes too. Plus rolled back the logging change since that is out-of-scope.

20th’s picture

Adding tests mentioned in #37. Now the file_save_upload_from_form() function should have almost complete test coverage. And I've also rearranged the names and order of variables to my liking.

Since SaveUploadFormTest testset is based on SaveUploadTest, there is a lot of duplicated test code now that will have to be cleared at some point.

20th’s picture

Forgot to fix some text typos…

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.

dmsmidt’s picture

Status: Needs review » Reviewed & tested by the community

@20th thanks a lot! Looks great. I manually tested again after your rearranging. And All tests @alexpott asked for in #37 are covered.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. As far as I can see we're still not testing that the error messages are actually attached to the form element and are not the regular drupal_set_messages() from file_save_upload(). Not sure of the best way of asserting this. Maybe by just installing inline_form_errors as part of the test?
  2. +++ b/core/modules/file/file.module
    @@ -701,21 +701,19 @@ function file_cron() {
    +  $errors_before = drupal_get_messages('error', TRUE);
    ...
    -  $new_errors = array_diff(drupal_get_messages('error'), $errors_before);
    +  // Get new errors that are generated while trying to save the upload.
    +  $errors_new = drupal_get_messages('error', TRUE);
    

    Removing this array diff is didn't look correct. However looking at the code $errors_before = drupal_get_messages('error', TRUE); will remove all the errors - I think this needs a comment. Also I think that the later call $errors_new = drupal_get_messages('error', TRUE); also should have a comment. It is pretty hidden that these calls reset the messages.

  3. +++ b/core/modules/file/src/Tests/SaveUploadFormTest.php
    @@ -362,4 +362,65 @@ public function testDrupalMovingUploadedFileError() {
    +   * Test that multiple validation errors are combined in one message.
    

    The core standard here is "Tests..."

  4. +++ b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
    @@ -114,6 +129,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +    drupal_set_message(t('Number of error messages before validation: @count', array('@count' => count($messages_before['error']))));
    +    foreach ($messages_before['error'] as $key => $message) {
    +      drupal_set_message(t('Error message @key before validation: @message', array('@key' => $key, '@message' => $message)));
    +    }
    +
    +    drupal_set_message(t('Number of error messages after validation: @count', array('@count' => count($messages_after['error']))));
    +    foreach ($messages_after['error'] as $key => $message) {
    +      drupal_set_message(t('Error message @key after validation: @message', array('@key' => $key, '@message' => $message)));
    +    }
    

    We shouldn't be outputting the actual error message here - we should just be asserting that the expected messages exist in the test. The message counts are fine but the actual message is a bit weird.

+++ b/core/modules/file/src/Tests/SaveUploadFormTest.php
@@ -0,0 +1,426 @@
+    $error = $this->randomMachineName();

There's no reason to use random text here - just make up an error message.

alexpott’s picture

Status: Needs work » Needs review
FileSize
28.98 KB
34.66 KB

re #42
1. I missed \Drupal\file\Tests\SaveUploadFormTest::testUploadFieldIsHighlighted() so this is actually covered. Nice one @20th!
2. Fixed
3. Fixed and the rest of the tests in the new test file - this is an error in the test we copied from.
4. Fixed
5. Fixed

Also fixed up array syntax stuff because it's changing soon. And made the test form do injection of dependencies.

alexpott’s picture

xjm’s picture

Title: File upload errors not set and shown correctly. » File upload errors not set or shown correctly
20th’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @alexpott! Can't catch up, you are way too fast.

The purpose of the testErrorMessagesAreNotChanged() test is to check that file_save_upload_from_from() does not discard any error messages by mistake; you are probably right and simply checking the number of errors before and after validation should be enough to test this.

alexpott’s picture

@20th the test also tests that the message is shown too - so it is not only asserting counts.

DamienMcKenna’s picture

Is there anyone we need to talk to about getting this committed? And what are the chances of getting it into 8.3 as well as 8.4?

SKAUGHT’s picture

@damien
this is targeted in the 'stabilze IFE roadmap' for 8.3 see: #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

xjm’s picture

@DamienMcKenna, it is not helpful to ask if there is "any chance" of getting this or any RTBC committed. That is really discouraging and frustrating to committers who are working around the clock (almost literally) to get caught up on a long RTBC queue with reduced resources from last year and lots of contributions from the GSW to review.

All RTBC issues will receive a response from a core committer as soon as they have the availability. This has been true in Drupal core for as long as I have been contributing (many years).

I promise, this issue is a very high priority to us because of its impact on IFE, as I outlined and explained on #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE). Also as I explained on that issue, it is eligible for 8.3.x backport up until the beta freeze deadline on Tuesday Feb. 14, but only after it has a committer's signoff and has been committed to 8.4.x. I will make every effort to review this patch before then if another committer does not, but there are also lots of other major and critical issues in the queue vying for attention as well as all the work to actually create the release itself.

Please be more considerate of the fact that even if you don't see the results of committers' contributions, they are going above and beyond to do what they can to make sure valuable contributions like this issue receive the thorough review and priority they do deserve.

xjm’s picture

Also pinged @tim.plunkett for form subsystem maintainer review of this fix if he is available. It's not a hard blocker for the issue, but would help me review it with confidence.

Edit: technically this is a file module issue, but that module has no maintainer, and the bug/workaround is for its form interaction/implementation. :) Thence not a hard blocker.

tim.plunkett’s picture

Taking a look. I have some concerns, but possibly not blockers.

xjm’s picture

@tim.plunkett reviewed the patch and @alexpott and I discussed his feedback.

This issue is a "Should"-have for IFE rather than "Must"-have, so if 8.3.0 ships without it fixed for IFE, that's non-ideal but manageable. We would like to fix it if possible because of the severity of the problems file module is causing for IFE here. But if this does not make it in time to possibly be backported, we can still identify it as a known issue with IFE, fix it in 8.4.x, and possibly discuss a patch-safe backport for IFE after we fix it in the best way.

Thanks, all! I am really encouraged that this is the last major core issue interfering with IFE.

tim.plunkett’s picture

I did not read all the test coverage, sorry.

  1. +++ b/core/modules/file/file.module
    @@ -674,6 +674,81 @@ function file_cron() {
    +function file_save_upload_from_form($element, FormStateInterface $form_state, $delta = NULL, $replace = FILE_EXISTS_RENAME) {
    

    I am surprised to see a new procedural function being added, especially one with this much trickery needed. I can understand the goal of getting this in now, but do we have a plan for making this less terrible in the future?

  2. +++ b/core/modules/file/file.module
    @@ -674,6 +674,81 @@ function file_cron() {
    +  $result = file_save_upload($upload_name, $upload_validators, $upload_location, $delta, $replace);
    
    @@ -710,6 +785,12 @@ function file_cron() {
     function file_save_upload($form_field_name, $validators = array(), $destination = FALSE, $delta = NULL, $replace = FILE_EXISTS_RENAME) {
    

    Looking at the rest of file_save_upload(), not pictured here, this is where all the terrible drupal_set_messages() are.
    Would it be worth it to inline that whole function here and skip the drupal_get_messages() part? Yes, we'd have to maintain the code in two places, but it might be better to write the future desired version now.

  3. +++ b/core/modules/file/file.module
    @@ -674,6 +674,81 @@ function file_cron() {
    +  // Get new errors that are generated while trying to save the upload. This
    +  // will also clear them from $_SESSION.
    +  $errors_new = drupal_get_messages('error');
    +  if (!empty($errors_new['error'])) {
    +    $errors_new = $errors_new['error'];
    

    Does our new proposed replacement for drupal_set_messages have a better mechanism for this? A follow-up on that issue would be good to see.

  4. +++ b/core/modules/file/file.module
    @@ -674,6 +674,81 @@ function file_cron() {
    +    if (count($errors_new) > 1) {
    +      // Render multiple errors into a single message.
    +      $render_array = [
    +        'error' => [
    +          '#markup' => t('One or more files could not be uploaded.'),
    +        ],
    +        'item_list' => [
    +          '#theme' => 'item_list',
    +          '#items' => $errors_new,
    +        ],
    +      ];
    +      $error_message = \Drupal::service('renderer')->renderPlain($render_array);
    +    }
    +    else {
    +      $error_message = reset($errors_new);
    +    }
    

    Okay now I just need to reread the issue, because I'm not sure why this hoop jumping is even needed.

  5. +++ b/core/modules/file/src/Tests/SaveUploadFormTest.php
    @@ -0,0 +1,460 @@
    +class SaveUploadFormTest extends FileManagedTestBase {
    

    Yuck for adding a new webtest, but that's how the class hierarchy works out I guess.

  6. +++ b/core/modules/file/src/Tests/SaveUploadFormTest.php
    @@ -0,0 +1,460 @@
    +    $this->assertTrue(is_file($this->image->getFileUri()), "The image file we're going to upload exists.");
    ...
    +    $this->assertTrue(is_file($this->phpfile->uri), 'The PHP file we are going to upload exists.');
    ...
    +    $this->drupalPostForm('file-test/save_upload_from_form_test', $edit, t('Submit'));
    

    Hm, I can't say I've *ever* seen stuff like this in a setUp() before. It feels wrong to assert things outside a test method.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs subsystem maintainer review

Thanks @tim.plunkett! Setting NR for addressing that review; @alexpott has some ideas on it and we are seeing what we can do.

alexpott’s picture

So here's a first cut of a variation on the idea. This refactors the main functionality of file_save_upload() into a trait. This new method accepts a param where the errors can be stored. The old file_save_upload() just does the usual drupal_set_message() stuff and the there is another method on the trait that sets the form errors. This new approach fixes a bug in the #43 because there is one message set that is not an error. The follow code would not have worked with the exact all the messages technique.

    // Rename potentially executable files, to help prevent exploits (i.e. will
    // rename filename.php.foo and filename.php to filename.php.foo.txt and
    // filename.php.txt, respectively). Don't rename if 'allow_insecure_uploads'
    // evaluates to TRUE.
    if (!\Drupal::config('system.file')->get('allow_insecure_uploads') && preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->getFilename()) && (substr($file->getFilename(), -4) != '.txt')) {
      $file->setMimeType('text/plain');
      // The destination filename will also later be used to create the URI.
      $file->setFilename($file->getFilename() . '.txt');
      // The .txt extension may not be in the allowed list of extensions. We have
      // to add it here or else the file upload will fail.
      if (!empty($extensions)) {
        $validators['file_validate_extensions'][0] .= ' txt';
        drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', array('%filename' => $file->getFilename())));
      }
    }

We need to add test coverage of this.

Re #54

  1. The new approach addresses this because the trait much more like we expect the future to look.
  2. The new approach means we still only have one code path but the trickery has gone.
  3. Not relevant anymore - no messing with messages.
  4. Not sure I understand what you are getting at. The reason dsm() is being used in file_save_upload is that you can't set multiple errors per form element.
  5. Yeah it is ugly but it is just how it works out atm
  6. We should open a followup to make the new test and the test it is copied from - \Drupal\file\Tests\SaveUploadTest way better. Yes this setUp thing is very very weird.

Of course this just means the patch gets bigger...

alexpott’s picture

Oops something from another issue ended up in #56. The interdiff is correct.

20th’s picture

So since this patch is being rewritten anyway, I'd like to bring up again what I've said in #31 – why not create a full service instead of a trait?

xjm’s picture

FWIW adding a new webtest is fine here; the initial mass conversion can exclude it if it blows up.

This seems to be tagged "Needs rests"; is that still true?

xjm’s picture

Issue tag field is very mobile-unfriendly. This needs followup for point 6 of Tim's review and will need a CR update once we agree on the update.

alexpott’s picture

@20th you're right a service is a great way forward and since @xjm has said that we don't have to fix this to keep IFE in core we can take our time to do this right. So we can also move the other file_*save* functions to it. In fact if we do that in another issue then this issue can add the necessary method to fix IFE's problem. I think we should do this in 3 steps.

  1. Create service by copying the methods into a service and naming the new methods right but minimising all internal method change - so allowing calls to procedural methods etc...
  2. Refactor the the service to use dependency injection and remove all deprecated function use.
  3. Fix this issue

That way each step is reviewable without a tonne of headaches.

alexpott’s picture

@20th I see you've kind of started on step 1 already in #2244513: Replace old managed and unmanaged file APIs with testable services. But this is combining step 1 and step 2 somewhat and introducing a whole new set of exceptions. I think first we need to create a service (or services) that do what the current methods do. We should ensure the interface reflects the need to do future work to separate UI from API functionality. All of the existing methods mix drupal_set_message() with API functionality making it really hard to disentangle everything.

dmsmidt’s picture

Status: Needs review » Postponed
Related issues: +#2244513: Replace old managed and unmanaged file APIs with testable services

There is already a follow up for IFE: #2687249: Improve inline form errors for file upload fields.

Let's postpone this on #2244513: Replace old managed and unmanaged file APIs with testable services, and first focus on steps 1-2 (#61).

xjm’s picture

My initial reaction is would actually prefer we hotfix IFE (backporting to 8.3.x) in as minimally disruptive a way as we can manage, and then refactor in 8.4.x followup, which we can have more confidence in now as a way forward. In general, we don't block major bug fixes on API rewrites unless the alternative is really untenable. It's also not in scope for IFE to clean up the whole file module.

The bug is still an accessibility issue for IFE, and so we still should fix it if we can before making IFE RC or stable. We just have the option to leave it outstanding if it's infeasible or onerous to fix it.

I also don't want to drain IFE resources into rewriting the file module from fixing other required issues in IFE itself, like disabling IFE with Quick Edit and other accessibility bugs.

I'll have a look and discuss more how much a hotfix and followups would work vs. blocking this fix.

dmsmidt’s picture

@xjm, thanks for that considerate answer. So maybe we can work on #57 as a hotfix, and in 8.4.x refactor the file module.
I'll keep this postponed then until your discussion results in a clear road map.

alexpott’s picture

I think the only part this is blocked on is deciding what should happen to file_save_upload() and doing that. The problem is deciding what to do with file_save_upload() needs to be done in concert with deciding how we approach all the work described in #2244513-46: Replace old managed and unmanaged file APIs with testable services. The problem with #57 is that it introduces something new to fix this bug without considering the wider system. And of course the really tricky thing is that IFE exposes the problem but the fix is not in IFE. I can't see how to fix this in IFE only.

xjm’s picture

@alexpott, but could we hotfix file.module in an internal way for 8.3.x, and then do the proper full fix in 8.4.x? by inlining code, marking any added API @internal and @deprecated for removal in 8.4.0, etc.

dmsmidt’s picture

@alexpott #66, Just for completeness, even without IFE the file field error handling is broken and sometimes confusing.

1) field not highlighted
2) in case of a required field, an extra non-helpful message is shown.

(See screenshots)

To fix this only for 8.3 and rethink the bigger picture in 8.4 we can temporarily go back to the 'tricky wrapper function' right? It should clearly be marked internal. As we won't want it to be adopted in contrib.

alexpott’s picture

@dmsmidt @xjm: I guess we could change file_save_upload_from_form to _file_save_upload_from_form() make it internal and not deprecate file_save_upload(). We would then have no recommended way for contrib usages of file_save_upload() to work with IFE in 8.3.x but that might be a path forward. Better than doing #57 without having a view on the much larger piece of work anyhow. But it doesn't get around the fact that for IFE to get stable we'd still need a non internal path for contrib to use.

xjm’s picture

Status: Postponed » Needs work

Yep, I agree that it will block IFE from being stable because of the contrib support problem. But for IFE to be beta in 8.3.0, backporting an underscore-namespaced workaround for core sounds like a good interim fix. The underscore-named function can have an @todo to the services issue and a note that contrib may not be fully supported until the followup is fixed. We can also mark it @internal and @deprecated explicitly to make it really clear it is not a full solution and will not necessarily be available in later minors.

Then we can mark IFE RC in 8.4.x once the file services issue and other issues on the roadmap are complete.

So, setting NW for a new patch (based on #43 probably instead of #57) that will include that backportable hotfix.

Thanks @dmsmidt and @alexpott!

xjm’s picture

I think we already will need to document that not all modules are fully supported yet anyway for #2828092: Inline Form Errors not compatible with Quick Edit . Maybe we should split that out into a separate documentation issue for hook_help() and maybe a hook_requirements() info message, since it applies to multiple aspects of IFE but can go in during beta as just a string addition.

dmsmidt’s picture

@xjm I think we should document that IFE can't be turned off per form easily. Quick Edit is a valid case where you want to turn it off, because it is a total different UI concept.
In the issue you mention we can turn IFE off for QE with a temporary fix. We would need a datamodel change to allow others to disable IFE. Or is a (form/renderelement) datamodel addition still possible in 8.3.x? Let's discuss it there.

On topic: if contrib uses the standard file widgets, they are compatible with IFE. Because this issue should fix the pressing concerns. Only if contrib uses the file_save_upload function directly they are out of luck. What I have seen so far is that when they do so, they mostly do so after form validation, which shouldn't be done anyhow, because the user can't fix a possible problem anymore in that stage. So we have an edge case that if contrib uses that function during validation they won't get a highlighted field and IFE doesn't look smooth.

DamienMcKenna’s picture

@xjm, others: I am sincerely sorry for my impatience in #48 :-(

dmsmidt’s picture

I sat with @xjm at the Drupal Dev Days and talked about how the previously mentioned issue #2244513: Replace old managed and unmanaged file APIs with testable services affects the stability of IFE.
We agree that it is a requirement to provide contrib with a nice API for file uploads that works with IFE.
However as long as contrib uses the field upload widgets provided by core, contrib will have working IFE, jay (and this patch also benefits installs without IFE)! Only if a custom upload widget is created, contrib would have to take care of IFE on its own.

So we are still on track of fixing this internally. @alexpott which patch has your preference of finishing? #43 or #57?
On the one hand I would prefer the smaller patch in #43 since we will do a major cleanup later. And marking the trait in #57 as internal seems strange. But then we need to fix the bug you identified in #56, and decide how we will cope with the notes from @tim.plunkett.
In any case we will need to mark the quickfix as internal add test coverage and move the assertions in the test setup out in a follow up (point 6 of #56).

Follow up on #72, I just want to note that we now have some plans for how to allow disabling IFE on certain forms. #2856950: Add a possibility to disable inline form errors for a complete form

dandaman’s picture

Trying to push this forward if possible and see a few issues. Patches use old array syntax so they won't apply to 8.4.x. Also, looks like the Theme Settings Form has been changed a bit, so that part may need to be reworked. I'm sprinting a bit at MidCamp so may try to get a bit more done in the next hour or two, but that is what I have identified so far. Obviously, as noted above, there's still a few different ways this could go, but I feel like trying to re-roll those patches may be somewhat helpful.

dandaman’s picture

OK, here's my patch that attempts at re-rolling #43. It seems to work on my test install but I'm certainly open to feedback, thanks.

mgifford’s picture

Status: Needs work » Needs review

Let's get the bots involved.

mgifford’s picture

The latest patch doesn't quite look quite like it does in the "After" photo in the issue when testing it out in node/add/article

image of errors when uploading an image with the wrong field type.

It works, but isn't quite as nice. The message "1 error has been found: Image" isn't going to be useful as it isn't at the top of the page, but right above the image upload.

Would it be useful to test both an image and file upload before this gets marked RTBC?

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

Assigned: dmsmidt » Unassigned

@dandaman, thanks for the reroll, but some parts where missing (#76). Here is another reroll.

@mgifford, the screenshots are without IFE. That the summary is shown is another issue (generic for all AJAX replaced form elements), we agreed in the UX meetings it can be removed. And yes both image and file fields should be checked.

dmsmidt’s picture

mgifford’s picture

I just retested this with the IFE enabled and I'm getting the same effect as I described in #78 with the latest patch.

It's not bad, it's just different than what was described in the initial description for this issue. I've included screenshots with/without the patch, both with IFE enabled.

dmsmidt’s picture

Issue summary: View changes
Status: Needs review » Needs work

@mgifford, we all wish an even better experience for IFE, thats why there is: #2876321: Update inline form errors summary after form validation with AJAX.
The focus here is fixing core (even without IFE), and let IFE show inline errors. I've added your screenshot to the IS.
Note we still need to work on the things mentioned in #74.

Edit: found we have a better follow up.

dmsmidt’s picture

dandaman’s picture

Patch in #81 seems to work for me.

The "null" error no longer shows. And without IFE, the error does show at the top of the form. Also, when the field is not required, the field is still marked red to denote the error. And when required, it does not show the null message.

mgifford is right that when Inline Form Errors (IFE) is enabled, it does look like above and maybe should have a more traditional red background message area or something. But maybe that is to be addressed in #2876321: Update inline form errors summary after form validation with AJAX now?

dmsmidt’s picture

@dandaman, correct, we are cleaning up Core here and doing our groundwork. We have that follow up for improving IFE display.

dmsmidt’s picture

Issue tags: +sprint
Sutharsan’s picture

Assigned: Unassigned » Sutharsan

Start working to add a functional JS test.

Sutharsan’s picture

Issue summary: View changes
Sutharsan’s picture

Uploading intermediate result: One functional JavaScript tests that checks that error is placed inside the field wrapper.
Needs:
- Move test to IEF module
- Clean-up

Sutharsan’s picture

Assigned: Sutharsan » Unassigned
Status: Needs work » Needs review
FileSize
3.45 KB
38.09 KB

Cleaned-up and more in line with existing IFE tests.
Ignore the previous patch and interdiff. The were only posted to save the intermediate results.

Any more tests needed for this issue?

Status: Needs review » Needs work

The last submitted patch, 91: 2482783-91.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dmsmidt’s picture

Thanks for working on this @Sutharsan, we had a good day!

Sorry for all the feedback, but here it goes!

  1. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +  /**
    +   * An user with administration permissions.
    +   *
    +   * @var \Drupal\user\UserInterface
    +   */
    +  protected $adminUser;
    

    Remove (not used).

  2. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $storage_settings = [
    ...
    +    ];
    ...
    +      'settings' => $storage_settings,
    +      'cardinality' => $storage_settings['cardinality'],
    

    This looks sketchy, I think we only need 'cardinality' => 1 in the create() method call.

  3. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $field_settings = [
    +      'required' => TRUE,
    +      'file_extensions' => 'png gif jpg jpeg',
    +    ];
    ...
    +      'settings' => $field_settings,
    

    'settings' => ['file_extensions' => 'png gif jpg jpeg'];

  4. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $field_settings = [
    +      'required' => TRUE,
    +      'file_extensions' => 'png gif jpg jpeg',
    +    ];
    

    No need for a separate variable. You can put it directly in the arguments.

  5. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +      'required' => $field_settings['required'],
    

    Just use TRUE.

  6. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $widget_settings = [];
    ...
    +      'settings' => $widget_settings,
    

    Remove.

  7. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    ])
    +    ->save();
    

    Chain without line break.

  8. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +
    

    Remove empty line.

  9. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $this->createScreenshot('/tmp/functional-test/file-upload.jpg');
    

    Remove.

  10. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $edit = [
    +      'edit-title-0-value' => $this->randomString(),
    +    ];
    +    $this->submitForm($edit, t('Save'));
    

    We should not submit the form, but just upload a wrong file format. And wait for AJAX to finish. The current test doesn't need to be a JavaScript test but could just be a browser test. We do want to test the validated AJAX response.

  11. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $selector = '.field--name-field-ief-file .form-item--error-message strong';
    +    $actual = $this->getSession()->getPage()->find('css', $selector)->getHtml();
    

    Could use a simpler selector in combination with getText().

  12. +++ b/core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerFileUpload.php
    @@ -0,0 +1,117 @@
    +    $this->assertTrue($actual == (string) t('@name field is required.', ['@name' => 'field_ief_file']));
    

    No need for translating.
    Use assertEquals.

Jo Fitzgerald’s picture

  1. Removed $adminUser.
  2. 'Inlined' variable.
  3. 'Inlined' variable.
  4. 'Inlined' variable.
  5. 'Inlined' variable.
  6. 'Inlined' variable.
  7. Removed line break.
  8. Removed empty line.
  9. Removed createScreenshot().
  10. How do we wait for the AJAX to finish?
  11. Simplified selector (can this be further simplified?).
  12. Removed t(), used assertEquals().

N.B. #93.10 is stil outstanding

Jo Fitzgerald’s picture

Status: Needs work » Needs review
dmsmidt’s picture

Assigned: Unassigned » dmsmidt
Status: Needs review » Needs work

@Jo thanks! I'll work on #93.10 and some others outstanding issues with this patch.

dmsmidt’s picture

dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Needs review
FileSize
18.19 KB
38.27 KB

Patch fixes #93.10 and applies the internal/deprecated notations.

To summarize:

  • It has been agreed (@xjm, @alexpott, @20th) that we fix this issue with minimally disruptive changes in order to get Inline Form Errors stable. And by doing so we actually fix two outstanding core bugs (see IS).
  • The above has been decided because the File API's will need to be converted to services anyhow and IFE should not have to wait for that, see below.
  • The temporary solution is the internal, deprecated wrapper function _file_save_upload_from_form() for file_save_upload which will be used in any form context. There was a concern for contrib, but this has been addressed in #74. file_save_upload will not be touched.
  • We have test coverage for the above. However these tests are largely copy pasted and should be cleaned up. We have follow up #2895431: Remove duplicate test coverage for _file_save_upload_from_form() and convert to Browsertest for that.
  • The not so helpful IFE summary will be addressed here #2876321: Update inline form errors summary after form validation with AJAX.

There are some outstanding concerns:

  • #54, @timplunketts addresses the trickery needed to get this fixed, and the introduction of a new procedural function. But could these be acceptable in the light of the above?
  • #56, @alexpott said there is a bug in the (non-trait fix) approach concerning non-error messages that are thrown in file_save_upload. I tested it manually but these messages show up as expected. So I can't write a failing test for that.

I think to move forward we have to move the discussion of how to refactor the file system out of this issue and start a new meta/plan issue for that, we could use #61 as a starting point and see how the work in #2244513: Replace old managed and unmanaged file APIs with testable services fits in. One of the tasks would be to get rid of all drupal_set_messages() and remove the internal function introduced here.

dmsmidt’s picture

Removing tags. We don't change existing API's and don't add public API's.
Also note that this issue does not solve duplicate error messages, see: #2346893: Duplicate AJAX wrapper around a file field.