Now that #1973436: Provide config schema to field types storage for image module has been committed work on #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct has revealed bugs in image_field_instance_update()

  // The value of a managed_file element can be an array if the #extended
  // property is set to TRUE.
  $fid_new = $field_instance->settings['default_image']['fid'];
  if (isset($fid_new['fids'])) {
    $fid_new = $fid_new['fids'];
  }
  $fid_old = $prior_instance->settings['default_image']['fid'];
  if (isset($fid_old['fids'])) {
    $fid_old = $fid_old['fids'];
  }

  // If the old and new files do not match, update the default accordingly.
  $file_new = $fid_new ? file_load($fid_new[0]) : FALSE;

So if the default image fid is not an array but just a scalar then accessing it using $fid_new[0] is completely wrong. #2130811: Use config schema in saving and validating configuration form to ensure data is consistent and correct it an integer using $fid_new[0] returns NULL and blows up Drupal\image\Tests\ImageFieldDefaultImagesTest - if it is a string (as it is now) it returns the first character - so if the fid is 10 it would load the file with fid =1 !!!!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
888 bytes

Initial patch...

vijaycs85’s picture

Issue tags: +Needs tests

We need a test with fid > 9 digit...

alexpott’s picture

So here is a way of testing this - just creates 10 files before creating the file used in the ImageFieldDefaultImagesTest.

In fact we have multiple issues here :( All of the hook_ENTITY_TYPE_crud implementations are suspect.

image_field_entity_delete() uses the array access on a string and will not delete file usage rows for multiple files if the default image used #extended = TRUE
image_field_instance_delete() will not delete file usage rows for multiple files if the default image used #extended = TRUE
image_field_instance_update() uses the array access on a string and will not update file usage rows for multiple files if the default image used #extended = TRUE
image_field_entity_update() will not update file usage rows for multiple files if the default image used #extended = TRUE

One massive issue is we have zero tests for image fields with multiple default images because the widget can not be configured to use #extended = TRUE.

Status: Needs review » Needs work

The last submitted patch, 3: 2155785.3.test-only-will-fail.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.25 KB

Correct patch for #3

The last submitted patch, 1: 2155785-image_field_instance_update-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2155785.4.test-only-will-fail.patch, failed testing.

alexpott’s picture

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

So the question here is should we keep the #extended functionality for images?

Here are two patches - one that removes it completely and one that fixes the hooks so that the bug reported by the issue title is fixed - but it does not fix the file usage part of the hook.

alexpott’s picture

One big issue with the use of #extended to store multiple fids in the default images fid field is this makes the schema impossible as the fid value is either an integer or an array.

If we want to support #extended then I suggest that we make it so the the fid is always an array. In the singleton case it should just contain one element.

But as noted there is no use for #extended in core for images SO we have a lot of work to do to make this happen.

Gábor Hojtsy’s picture

Who is #extended there for then? :) Who uses it?

The last submitted patch, 8: 2155785.8.keep-extended.patch, failed testing.

yched’s picture

I'm not sure why we would care about the #extended property for default_images.

Currently only the ImageWidget uses #extended = TRUE, because it does allow multiple uploads.

But the 'default image' input in field / instance configuration forms doesn't use ImageWidget, it uses a regular ad-hoc '#type' => 'managed_file' input, without #extended (see ImageItem::defaultImageForm()). Ergo, 'default image' cannot be an array of files, it can only be just one single file, and there should be no code trying to support the case where it can be an array.

Supporting multiple images for "default image" might be an existing feature request, but it's too late for feature requests :-) - and config schema would indeed make that difficult.

Status: Needs review » Needs work

The last submitted patch, 8: 2155785.8.no-extended.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
46.72 KB
2.52 KB

This is a mess.

At the moment if you create a default image it will save it like this:

  default_image:
    fid:
      - '1'
    alt: ''
    title: ''
    width: null
    height: null

For the complete file see https://gist.github.com/alexpott/f321a7a8c4b1fa31892d

So fid here is an array - which is not what the schema or \Drupal\image\Tests\ImageFieldDefaultImagesTest expects.

Patch attached creates a custom validator for the default image form to massage the value to be a scalar.

The last submitted patch, 14: 2155785.14.no-extended.patch, failed testing.

alexpott’s picture

FileSize
8.38 KB
2.52 KB

Wrong patch

yched’s picture

Thanks @alex - the extra massaging to unwrap the array is indeed a bit tedious, but it's best to model our data according to what makes sense for the domain logic and use cases we support rather than by what form structures and fapi elements dictate.

  1. +++ b/core/modules/image/lib/Drupal/image/Plugin/Field/FieldType/ImageItem.php
    @@ -335,6 +335,7 @@ protected function defaultImageForm(array &$element, array $settings) {
    +      '#element_validate' => array(array($this, 'validateDefaultImageForm')),
    

    For form serialization matters, better if validateDefaultImageForm is a static method, and added to the form with
    array(get_class($this), 'validateDefaultImageForm')

  2. +++ b/core/modules/image/lib/Drupal/image/Plugin/Field/FieldType/ImageItem.php
    @@ -361,6 +362,34 @@ protected function defaultImageForm(array &$element, array $settings) {
    +    file_managed_file_validate($element, $form_state);
    

    My FAPI is a bit rusty, but isn't there a way to set our #element_validate callback so that the default for the #type still runs ?
    (maybe there isn't)

sun’s picture

I was never really familiar with File/Image module, but if I remember correctly, support for multiple default images was added to D7, so as to allow contrib to implement and solve that use-case, whereas Drupal core only implements one default image.

In a sense, the feature request was not implemented in Drupal core, but is only supported by core. I only found this seemingly related issue: #1482386: Support multiple default values for fields with cardinality > 1

I guess, if we limit it to 1 in core, the question is whether a contrib module would be able to unlock that limit, so that the use-case of multiple default images can be supported (which is an edge-case, but IIRC, some good use-case examples were provided for that some time ago).

OTOH, do other fields support multiple default values? If they do, isn't the file/image field an alien and inconsistent by declaring the schema to only support a single value? Or is all of that the purpose of #1482386 ?

yched’s picture

I'm not familiar with that area and the intents & strategy of image field type features.
I only encountered this question of "array / not array" for default_value when I needed to work on a followup fix for the overly confusing #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute.

It looks like:
- The initial patch that got committed there in #166 moved 'default_value' to an array internally, without actual UI support for the feature.
- There was a problem with the corresponding upgrade, so me and a couple others struggled for a while to fix it as a followup in the issue.
- Then quicksketch #214 basically said "why did we turn that into an array, let's revert that", and they worked on a patch to revert it, along with the faulty upgrade function, so I stopped caring.
- But apparently they stopped somewhere in the middle, and the followup patch that eventually got in left a strange mix of support for array / non array - see #220 / #223.

do other fields support multiple default values ?

The basic functionality for fields "default values" supports multiple default values.
But Image fields are different, they opt out of the regular Field API notion of "default value" and associated processing, and they implement their own different thing.

- "Field API default values" are values that get prepopulated when you create a fresh $entity object.

- What Image field calls "default_value" is in fact a "fallback value", it only kicks in at display time. This is handled by a one-off Image-specific field setting that happens to be called 'default_value', but it's only a custom ad-hoc setting used by ImageFormatterBase.

alexpott’s picture

FileSize
2.17 KB
8.02 KB

Thanks for the review in #17 @yched

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reminder and explanation, @yched :)

I remember that construct now. Seems like it would make sense to rename the "default_image" key to "placeholder_image", so as to prevent this confusion in the future. I'm fairly sure that everyone thinks about prepopulated values in the form when reading "default" ;)

Since the placeholder image is used by the formatter only, I assume there are easy ways to supply a formatter that renders multiple placeholder images, so we can limit it to 1.

In turn, I guess we can move forward here?

yched’s picture

Seems like it would make sense to rename the "default_image" key to "placeholder_image"

Absolutely - that, or "fallback_image"...
I'm not going to make this happen, but I'd totally +1 it ;-)

catch’s picture

Status: Reviewed & tested by the community » Fixed

One of my favourite issue titles for some time. Almost a shame to close it so quickly.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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