Widget context and validators not passed to Entity Browser element

@arknoll I'm glad someone is following my issues in File Browser! I was worried that no one found any use out of that patch given the feedback I was getting.

Entity Browser Widgets can be used in many contexts (Entity Embed, as a form element eventually, other complex contrib use cases...), so we'll need to do this in a way that's field agnostic. They're not intended to be aware of how they're used or displayed to keep them generic.

When #2366335: Support defining conditions lands, I'm going to re-roll the File Browser issue I made to use EntityBrowserWidgetValidations (a new plugin type) so that this kind of validation can be generic and not tied down to fields. After that patch lands, I'll be working to port File Browser's Field Widget in #2763505: Move file_entity and file_browser's Field Widget for Files and Images to Entity Browser.

We paved the path for this in the two issues that @samuel.mortenson mentioned. Upload location should now be passed on and respected. Re-purposing this issue to add support for other field-specific configuration.

First step would be to create list of configuration variables that we need to pass on along the upload location.

I think the settings that are covered in this patch should all be respected with the new WidgetValidation and Field Widget work, at least from my manual testing.

Might be worth having @arknoll give the newest dev release of File Browser (with no patches) and Entity Browser a pass and see if things are working the way he expects. If so, this issue could be closed! If not we can use it to tweak the work already done in other issues.

Pending an answer to #7, here's an update to the patch to fix missing token replacement in the upload directory under certain circumstances. Setting back to "Needs review" just for the testbot.

Re-rolling patch for alpha7.

@jsdix What field settings are not respected in the alpha7 release using the FileBrowserWidget? The current patch is incompatible with how we currently perform validation and pass contextual settings to widgets.

@samuelmortenson these in particular we were attempting to pass to the file field,

    '#multiple' => $multiple,
      '#upload_validators' => [
        'file_validate_extensions' => [$contextual_settings['file_extensions']],
        'file_validate_size' => [$max_filesize],
      ]

I didn't see these added anywhere else, are they?

Missed a use statement in #9 patch. Attached is the new patch. Although still discussing with @samuelmortenson if attributes are being respecting in another way.

Putting this issue back into Needs Work as this is incompatible with how Entity Browser currently does File validation.

@jsdix You can see how validators get added in the current Entity Browser codebase here: http://cgit.drupalcode.org/entity_browser/tree/src/Plugin/Field/FieldWid...

Reading persistent data on form generation seems to do the job. It pushes the validation plugin ids generated by the widget to the entity browser and then, validation plugin are called on submit.

However, this simple solution has two things that bother me.

First, errors are not reported on the upload event but when clicking on the 'Select entity" button. Which means you can create a file entity invalid for the current use case.

I tried adding validation on upload by passing the validation functions to the upload form element :

$validators = $form_state->get(['entity_browser', 'validators']);

    $form['upload'] = [
      '#type' => 'managed_file',
      '#title' => $this->t('Choose a file'),
      '#title_display' => 'invisible',
      '#upload_location' => $this->token->replace($this->configuration['upload_location']),
      // Multiple uploads will only be accepted if the source field allows
      // more than one value.
      '#multiple' => $field_cardinality != 1 && $this->configuration['multiple'],
      '#upload_validators' => array_merge(
        ['file_validate_extensions' => [$this->configuration['extensions']]],
        $validators ['file']['validators']
      ),
    ];

but it produces a double validation and error messages.

The file module add an erro message and the entity browser validation widget as well. I didn't find a way to priviledge one over the other.

Finally, if an image is bigger than authorised, it is automatically resized and a message is display to informed the user with "drupal_set_message()". For an unknown reason, this message is displayed on file upload and appears twice (even if drupal_set_message is only called once).

Still needs work.

@lucaslg Thank you for the research and for not working based on the current patches. :-)

Looking at your patch, then at the source code for EntityReferenceBrowserWidget, there is clearly a regression as ::getPersistentData() is no longer used anywhere.

I bisected the commits and it looks like the introduction of the render element was responsible for removing this functionality. If you look at http://cgit.drupalcode.org/entity_browser/commit/?id=729d7dbaf8d4092536b... you can see that the getPersistentData() call was removed.

@slashrsm I'm bumping up the priority of this since, from my understanding, this hasn't worked in 8 months and is basic expected functionality.

I'm assigning this to myself and will work off of #18.

I closed #2763511: Pass field settings/configuration to EntityBrowserWidgets from the EntityReference FieldWidget and #2669788: Using the file/image field widget does not respect the upload directory of the field. as both issues concerned supporting field configurations in Entity Browser.

The duplicate error messages reported in #18 are a result of a core bug (#2346893: Duplicate AJAX wrapper around a file field), and are out of scope for this issue.

Here's a new patch which adds back widget context, with test coverage! Everything done here brings Entity Browser back to the state it was before the regression described in #20, where widget contexts can be passed from the highest level down to widgets to override arbitrary configuration at run time.

One change introduced here that's new is that the Upload widget has a new configuration setting, "upload_validators", which contains validators to pass to the managed file widget. This new configuration setting only exists to be overridden using widget context, which is a little awkward but is how the widget context system was intended to work. I added this to the schema as "ignore", which is usually not a great practice but for this specific use case makes sense, as "upload_validators" could contain any valid callback for the managed file widget.

I'd also like to apologize for rejecting the solutions in comments #7, #10, and #16. I was operating under the assumption that the widget context code I wrote was still functioning, without manually testing things on a more recent Entity Browser release. If I would have reviewed the codebase and found the regression earlier, I could have had this patch out seven or so months ago. If this gets committed I'd like to make sure that everyone involved gets credit. :-)

Tweaked things a bit to pass tests. I also excluded filesize and resolution validation from the View widget, as I didn't want to disrupt current behavior too much. The upload widget will have all validators run, but other widgets would have to opt-into full validation.

Removed debug code, improved upload specific validator logic.

I'm going to spend some time making sure #2808687: Use reusable settings defined on field level rather than duplicating configuration will also be solved by this issue, and roll a new patch. Tests are green!

I thought about things a bit and realized that widgets have a wide variety of configurations, and should be in control of how the widget context is handled. This patch adds a new method to WidgetBase, handleWidgetContext, which allows widgets to override the default behavior of widget context overriding configuration. You can see how this can be implemented in the patch I'll upload to #2808687: Use reusable settings defined on field level rather than duplicating configuration.

I also removed the upload_validators configuration key from the Upload widget's schema, as it was awkward and it made more sense to the Upload widget to access the context directly. Different widgets are going to use context differently, and that's OK.

This is the only other change I had planned, so I'd say this issue is ready for review.

I've had a go at respecting the upload_validators from the widget context in the View widget, when the entities in question are files. I really don't know if this is a good way to do it, but my client needs the minimum dimensions set on the file to be respected, even when selecting existing files - this was the best quick way I could come up with. I'd be interested to hear what a better approach would be though! Note that I haven't tested this with entities other than files (not even media entities), it should just get ignored (which wouldn't be ideal for media image entities).

Ideally, my view would be filtered to only show images within the minimum dimensions, but I don't think that information is exposed to views. (It's sat in a file_metadata table, one row per dimension, so isn't even nice to join in without duplicating results.)

That seems like a pretty irrelevant failure, not sure why that's happened?

@james.williams In my patch from #30 I specifically split upload-specific validators from other validators in FileBrowserWidget::getFileValidators.

If you'd like to make a change to how this works, it should be done when passing widget_context and validators (i.e. in FileBrowserWidget), not in the Entity Browser View widget. Otherwise we would end up hard-coding custom validation for multiple Entity Types in the View widget and things would get really messy.

I agree that minimum dimensions makes sense for a lot of use cases, I can add this in a new patch. In my opinion maximum dimensions should only be respected on upload, although the community might think differently. I should have something uploaded later today.

Thanks @samuel.mortenson , that would be amazing! Sorry I couldn't see the better way of doing it myself :-) Still not sure why the test is failing, but that's irrelevant if a better solution arrives of course.

I really do think max & min dimensions need respecting when selecting from the View widget, as the View will (usually) be listing images used across all/most image fields, each of which might have their own allowed dimensions. For example, one image field might intentionally be for high-resolution images, and so shouldn't allow small images originally uploaded to a 'teaser images' field or similar. I guess you could build a different view & browser for each field in that case, but that doesn't feel right either, as the entity browser is intentionally reusable across fields.

@james.williams No problem! With that feedback in mind, I guess we should apply all validators to all widgets, and not make opinions about who wants what. This should clear up the patch a little bit, although I could see users getting upset that they can't select existing File Entities because they don't meet a field's requirements for resolution.

Maybe as a follow up we can discuss adding some APIs or Entity Browser configurations that allow users to exclude certain validators per-widget. I'll consider this today as I work on a new patch. Lots of options!

Here's a new patch which validates resolution for existing files as well as uploads. @james.williams let me know if this works for you, or if you have any other feedback.

@samuel.mortenson that is fantastic - thank you so much!! My client will be very pleased that this is solved :-)

I filed #2865928: Provide method for views widget to filter based on context as a follow-up as well, for watchers that want to follow that issue.

I think it's not completely working.
The resolution validation (maximum/minimum size) works pretty well. The problem is that after the error is shown, it seems to go in some inconsistent state.
As you can see in the screenshot, if I try to upload another image instead of the incorrect one, under the name of the new one there's still the upload button.
If I try to save that image it returns the following error:

Argument 1 passed to file_validate() must implement interface Drupal\file\FileInterface, null given, called in /var/www/project/docroot/modules/contrib/entity_browser/src/Plugin/EntityBrowser/WidgetValidation/File.php on line 31 and defined in file_validate() (line 286 of core/modules/file/file.module).

@sanci What module are you using that provides the Entity Browser in your screenshots? I can't replicate this using just the Entity Browser test sub-module.

Edit: I see now that it's Lightning Media, but I can't replicate the error. I'll try digging a bit more.

@sanci I've created a new issue for Lightning (#2867473: Support Image/File field settings when using the Lightning Media browser) so that we can take this discussion there. Please test the patch there (after applying this patch to Entity Browser) and post feedback in #2867473: Support Image/File field settings when using the Lightning Media browser.

This patch makes perfect sense. I'm going to try to bring it into Lightning as soon as possible.

1. +++ b/src/Plugin/EntityBrowser/Widget/Upload.php
   @@ -107,9 +108,9 @@ class Upload extends WidgetBase {
   +      '#upload_validators' => array_merge([
   

   Perhaps this should be array_merge_recursive() or NestedArray::mergeDeep()?

2. +++ b/src/WidgetBase.php
   @@ -361,4 +354,22 @@ abstract class WidgetBase extends PluginBase implements WidgetInterface, Contain
   +  protected function handleWidgetContext($widget_context) {
   

   $widget_context should be type hinted.

3. +++ b/tests/src/FunctionalJavascript/ImageFieldTest.php
   @@ -139,4 +151,46 @@ class ImageFieldTest extends EntityBrowserJavascriptTestBase {
   +  public function testImageFieldSettings() {
   

   For readability, it might make sense to call $this->getSession() once at the top of the method. That's a nitpick, though.

I pulled this patch into Lightning and it successfully passed our tests. We were doing some very similar work (now obviated by this patch) and thoroughly testing it. If it's good enough for Lightning, and passing its fairly rigorous tests, I feel quite confident that this is RTBC-quality. The nitpicks in my previous comment, if anyone wants to address them, are probably minor enough to fix on commit.

Here's the pull request that brings this into Lightning, for those who may be curious: https://github.com/acquia/lightning/pull/336

Great stuff! Committed.

With respect to the issue https://www.drupal.org/project/entity_browser/issues/2669788 which was closed as a duplicate of this issue.

It's still not clear how the upload location of the referenced field can be used instead of the 'Upload location' field in the entity browser settings.

Can someone point me in the right direction?

I've looked through the patches but nothing seems to clarify this.

Adam,

I realize this is a late reply, but I experienced the same confusion, and want to post this here for others that stumble upon it.

The data from the field is stored in the form_state storage. It's in an array like:

$form_state->storage = [
  'entity_browser' => [
    'widget_context' => [
      'upload_location' => 'public://',
      'upload_validators' => [...],
    ],
  ],
];

From your entity browser widget, you can get the values like:
If you want the widget context: $form_state->get(['entity_browser', 'widget_context']);
If you want the upload location: $form_state->get(['entity_browser', 'widget_context', 'upload_location']);