Required croppings

Hi @Sander Hofman

It's totally true, ATM it's impossible to require a crop :O That is not really hard to implement ! I can purpose a patch to assume this feature but I need you for define our real needed about it.

We add this option onto 'widget setting' as 'Crop required' to force user's to define a crop zone to save an entity ? We do add a check onto entity_insert/update basis or more hight level in form_element (I suppose yes...). Can we discuss more for not forget other implications about that ? These option do not be activate in same time of future Automated Crop or we just can add a check to ensure we have an automatic_crop configured onto style collection and our image are mendatory cropped.

Here's a patch that made all selected crops required.

It's a quick and dirty one : code reviews and unit tests are welcome

Needs a bit more work if you want only some of the crop to be mandatory.

A part of fails are fixed by #2887301: Fixe 2.x branch tests but I need to test more to tell you what is the problem here.

I re-run the test now tests are fixed.

re-roll correclty ...

Add a missing FieldWidget file

Hi, thank you for this patch it can be very useful.

On the other hand the control of the mandatory crop must be done with the backup of the node and not with the upload of the file, a small patch to correct this case.

Hi,

After discussion with @woprrr I tried to optimize the code. Please let me know if there are any other changes.

Thx

The patch isn't applicable onto 8.x branch please update your patch.

Thank @salla I review fast this patch

Hi,

I reroll the patch, thank @woprrr for help.

Patch #17 works on image_wdiget_crop 2.0.0. Thanks.

1. +++ b/config/schema/image_widget_crop.schema.yml
   @@ -27,6 +27,9 @@ field.widget.settings.image_widget_crop:
   +      label: 'Crops are required to validate file'
   

   I think the following wording are more clear : "Set Crops as required"

2. +++ b/config/schema/image_widget_crop.schema.yml
   @@ -58,3 +61,6 @@ image_widget_crop.settings:
   +          label: 'Crops are required to validate file'
   

   same feedback about wording.

3. +++ b/src/Element/ImageCrop.php
   @@ -34,6 +34,7 @@ class ImageCrop extends FormElement {
   +      '#crop_required' => FALSE,
   

   Actually I have a doubts about this. I explain why at the bottom because we need to choose the better way to implement that (Crop basis of Widget Basis).

4. +++ b/src/Element/ImageCrop.php
   @@ -192,7 +193,10 @@ class ImageCrop extends FormElement {
   +              'data-drupal-iwc-id' => $type,
   
   @@ -402,6 +415,63 @@ class ImageCrop extends FormElement {
   +      ->load($element['#attributes']['data-drupal-iwc-id']);
   

   This is useless because $element already have some mentions of type like key($element['crop_wrapper']);

5. +++ b/src/Element/ImageCrop.php
   @@ -209,6 +213,15 @@ class ImageCrop extends FormElement {
   +            $element['crop_wrapper'][$type]['crop_container']['values']['crop_applied']['#element_validate'] = [
   +              [
   +                static::class,
   +                'cropRequired',
   +              ],
   +            ];
   

   We can increase the readability of code with only one line here.

6. +++ b/src/Element/ImageCrop.php
   @@ -402,6 +415,63 @@ class ImageCrop extends FormElement {
   +  public static function cropRequired(array $element, FormStateInterface $form_state) {
   

   setCropRequired(); seem better here.

7. +++ b/src/Element/ImageCrop.php
   @@ -402,6 +415,63 @@ class ImageCrop extends FormElement {
   +    if (self::fileTriggred($form_state) && self::requiredApplicable($crop_applied, $operation)) {
   +      $form_state->setError($element, t('Crop @crop_name is required.',
   +        [
   +          '@crop_name' => $crop_type->label(),
   +        ]
   +      ));
   +    }
   ...
   +  public static function requiredApplicable($crop_applied, $operation) {
   +    return ((int) $crop_applied === 0 && $operation !== 'Remove');
   +  }
   

   self::requiredApplicable sound better as self::isRequired

   This test works well in 80% of usecases but if I only need to set as required 1 of 3 crops available in this element I can't submit without save a crop in others elements. We need to identify where is the crop_type required like a boolean at CropType basis.

8. +++ b/src/Form/CropWidgetForm.php
   @@ -181,6 +181,12 @@ class CropWidgetForm extends ConfigFormBase {
   +    $form['image_crop']['crop_required'] = [
   +      '#title' => $this->t('Crops are required to validate file'),
   +      '#type' => 'checkbox',
   +      '#default_value' => $this->settings->get('settings.crop_required'),
   +    ];
   
   @@ -245,7 +251,8 @@ class CropWidgetForm extends ConfigFormBase {
   +      ->set("settings.crop_required", $form_state->getValue('crop_required'));
   
   +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -126,6 +127,7 @@ class ImageCropWidget extends ImageWidget {
   +          '#crop_required' => $element['#crop_required'],
   
   @@ -210,6 +212,12 @@ class ImageCropWidget extends ImageWidget {
   +    $element['crop_required'] = [
   +      '#title' => $this->t('Crops are required to validate file.'),
   +      '#type' => 'checkbox',
   +      '#default_value' => $this->getSetting('crop_required'),
   +    ];
   
   @@ -234,10 +242,12 @@ class ImageCropWidget extends ImageWidget {
   +    $crop_required = $this->getSetting('crop_required');
   ...
   +    $preview[] = $this->t('Crops are required to validate file: @bool', ['@bool' => ($crop_required) ? 'Yes' : 'No']);
   
   @@ -270,6 +280,7 @@ class ImageCropWidget extends ImageWidget {
   +    $element['#crop_required'] = $this->getSetting('crop_required');
   

   Here we really need to expose that configuration at ImageWidgetCrop widget basis ? If we attach this responsibility at CropType Basis we can delegate this here because if not we can't choose what crop type are required but all element are invalidate if we haven't all crops with values.

Actually we are in the good way to validate elements with required crops BUT we need to decide what is the best approach. If we force element to have ALL values completed to validate the element we risk to add more complexity in publish process.

I add @miro_dieker to this for have other thing about this possible border effect.

Updating to the canonical tag for DrupalCon Vienna 2017.

More specifications about solution Here #2912918: Possibility to define manual crop effect as required.

Solution 2 is the one in the patch because it seems the most flexible and easy to use. It would be tempting to place this information within the CropType entity but this implies that all consumers of this type crop are impacting out with the ImageStyles effects we enjoy a flexibility and a stronger abstraction layer as we can use the same CropType on several ImageStyles this is the same field definition vs field widget settings. This solution seems to me the most appropriate for a simple and compatible use with all CropAPI users.

This patch isn't finished because we need to reduce complexity and coding standards fast. But the basis is already here and functional. Please continue in this way ;)

After somes discuss with @slashrsm , We have conclude Crop API doesn't assume "required' field at Crop Type basis because this responsability do be assume by UI modules and not by Storage.

I change that in patch and migrate that behavior onto field Widget configuration. Like initial purpose.

Here a patch with the new way FieldWidgetBased to choose Required Crop by widget instance (Very powerfull for Site builder). Saddly I haven't more time to finish a small "refactor" of that If anyone can continue that's GREAT :)

The concept is to ensure we have all control of what Crop type is flagged require we have added a property (crop_types_required) to list on widget settings what crop_type are considered required for this field instance. For more simplicity and flexibility this list are populated by your choose onto previous element (crop_list) to be sure we required an active element and avoid complexity.

To test it we just need to choose what Crop type to apply to your image when you have selected your types the next list 'Set Crop Type as required' are populated with your crop types you can choose what crop type you need to be required to pass crop_image element validate.

Tomorow I will post a new screencast to show how that works. I need to have more feedbacks about that (code / usability) please.

I attach the patch to continue and add @TODO to illustrate what part I need to refactor.

1. +++ b/src/Element/ImageCrop.php
   @@ -142,7 +143,7 @@
   +          $is_required = $iwc_manager->getEffectData(current($style), 'crop_types_required');
   

   This part is now unecessary we need to retrieve this information with another way.

2. +++ b/src/Form/CropWidgetForm.php
   @@ -181,10 +181,15 @@
   +      '#options' => $this->imageWidgetCropManager->getAvailableCropType(CropType::getCropTypeNames()),
   

   Options need to be [] empty at the begining and populate by user choose in crop list element.

3. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -211,16 +230,78 @@
   +      [Select::class, 'processSelect'],
   

   Hummm obscure trick ... Halloween effect ?! We are mendatory to re-affect this process to save #multiple onto element if not add that the select are considered as #multiple => FALSE ...

4. +++ b/src/Plugin/Field/FieldWidget/ImageCropWidget.php
   @@ -211,16 +230,78 @@
   +  public function populateRequiredList(array $form, FormStateInterface $form_state) {
   +    $response = new AjaxResponse();
   +    $triggering_element = $form_state->getTriggeringElement();
   +    if (isset($triggering_element['#value'])) {
   +      $crop_type_required_form = $form_state->getCompleteForm()[$form_state->getTriggeringElement()['#parents'][0]][$form_state->getTriggeringElement()['#parents'][1]]['plugin']['settings_edit_form']['settings']['crop_types_required'];
   +      $crop_type_required_form['#options'] = array_intersect_key($triggering_element['#options'], $triggering_element['#value']);
   +      /** @var \Drupal\Core\Render\RendererInterface $renderer */
   +      $renderer = \Drupal::service('renderer');
   +      $element = $renderer->renderRoot($crop_type_required_form);
   +      $field_name = str_replace('_', '-', $crop_type_required_form['#parents'][1]);
   +      $target_id = ".form-item-fields-$field_name-settings-edit-form-settings-crop-types-required";
   +      $response->addCommand(new ReplaceCommand($target_id, $element));
   +    }
   +
   +    return $response;
   +  }
   

   We need to simplify this more and more ... Actually that works well but not really clear ...

Here I think we are in the good way :). I flag issue need review to have more feedback by anyone need to make feedback but I need to add Functional test to cover these feature and create a follow-up to cover ALL usecases not already covered.

@miro_dietiker If you have a small time It's possible to have a meeting or mails about your opinion for that feature. I prepare a screencast to show how that works.

Thanks all.

:D When you talk about Tests he's came :D

Here a video to show How Required Crop works and how configure Crops with this patch.

https://youtu.be/X6gmJ162E5o

Here you have some adjust to prevent all Miss configuration potential problems. I add that after a support session on Slack.

Here you are two adjusts to prevent miss configuration and UX improvements...

@woprr, at your request, some suggestions about wording. (Warning: a lot of this is very, very picky. I spent years as an ESL teacher and now I can't turn that part of my brain off. It's really terrible.)

1. +++ b/config/schema/image_widget_crop.schema.yml
   @@ -18,6 +18,11 @@ field.widget.settings.image_widget_crop:
   +      label: 'Crop Types required'
   

   @@ -49,6 +54,11 @@ image_widget_crop.settings:
   +          label: 'Crop Types required'
   

   • I would suggest instead "Required crop types" for these items, since the user is choosing which type(s) will be the required crop types.
   • This would also apply to some areas outside of the current patch so maybe not important now, but I just wanted to note that the Capitalization section of the Interface text page suggests using sentence case in these situations, i.e. "Required crop types" instead of "Required Crop Types".

2. @@ -183,13 +199,26 @@ class ImageCropWidget extends ImageWidget {
   +      '#title' => $this->t('Crop Types required'),
   +      '#type' => 'select',
   +      '#options' => $crop_types_options,
   +      '#default_value' => $this->getSetting('crop_types_required'),
   +      '#multiple' => TRUE,
   +      '#description' => $this->t('Make crop types selected required.'),

   • Same suggestion as above for "Crop Types required" -> "Required crop types".
   • "Make crop types selected required." -> "Crop types that should be required."

3. +++ b/src/Form/CropWidgetForm.php
   @@ -181,6 +181,17 @@ class CropWidgetForm extends ConfigFormBase {
          '#default_value' => $this->settings->get('settings.show_default_crop'),
        ];
    
   +    $form['image_crop']['crop_types_required'] = [
   +      '#title' => $this->t('Set Crop Type as required'),
   +      '#type' => 'select',
   +      '#options' => $this->imageWidgetCropManager->getAvailableCropType(CropType::getCropTypeNames()),
   +      '#empty_option' => $this->t("- Any crop selected -"),
   +      '#default_value' => $this->settings->get('settings.crop_types_required'),
   +      '#multiple' => TRUE,
   +      '#description' => $this->t('Set actives crop as required.'),

   • Capitalization thing.
   • "Set actives crop as required." -> "Set active crop as required."

4. @@ -169,8 +172,21 @@ class ImageCropWidget extends ImageWidget {
   +        '#markup' => $this->t('No Image Style using the "Manual Crop" effect found, please first go @link and attach "Manual Crop" effect, then return to configure the field widget settings.', ['@link' => Link::createFromRoute('configure one here', 'entity.image_style.collection')->toString()]),
   

   I'd suggest 'No image style using the "manual crop" effect found. Please first go @link and attach the "manual crop" effect and then return to configure the field widget settings.'

5. @@ -254,13 +391,17 @@ class ImageCropWidget extends ImageWidget {
   +      $preview[] = $this->t('Crop Type required : @list', ['@list' => implode(", ", $crop_required)]);
   

   The capitalization thing again.

I hope this helps! Happy to roll you a patch tomorrow with these changes incorporated, if that's useful to you.

Hey @bdimaggio :) THANK YOU for this wonderfull review ++++++

Here the patch with all fixes. Can you take a feedback about this interdiff ? :) Thank again !!

The test failling is "normal" this is due to Image Widget Crop Examples sub modules dependencies. Ctools / Media entity use deprecated methods , I have already purpose patch in modules issue queue to solve this.

We are using this patch in our project on production since a longer time. Please commit it into the next release.

Hey @woprrr, a belated RTBC from me too. Your wording/spelling/capitalization/etc looks great!

I have noticed, that I get a lot of PHP warnings from below (the same, multiple times), when I am on a media entity edit page e.g. /media/MEDIA-ID/edit:

Warning: in_array() expects parameter 2 to be array, null given in Drupal\image_widget_crop\Element\ImageCrop::isRequiredType() (line 266 of modules/contrib/image_widget_crop/src/Element/ImageCrop.php).
Drupal\image_widget_crop\Element\ImageCrop::isRequiredType(Array, '16_to_9') (Line: 150)
Drupal\image_widget_crop\Element\ImageCrop::processCrop(Array, Object, Array)
call_user_func_array(Array, Array) (Line: 982)
Drupal\Core\Form\FormBuilder->doBuildForm('media_image_edit_form', Array, Object) (Line: 1045)
Drupal\Core\Form\FormBuilder->doBuildForm('media_image_edit_form', Array, Object) (Line: 1045)
Drupal\Core\Form\FormBuilder->doBuildForm('media_image_edit_form', Array, Object) (Line: 1045)
Drupal\Core\Form\FormBuilder->doBuildForm('media_image_edit_form', Array, Object) (Line: 1045)
Drupal\Core\Form\FormBuilder->doBuildForm('media_image_edit_form', Array, Object) (Line: 557)
Drupal\Core\Form\FormBuilder->processForm('media_image_edit_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('media_image_edit_form', Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 137)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 57)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I think this needs fixing. Therefor I am opening this issue again.

Hi @Peter Majmesku and thank to this pragmatic review :) This is important to correctly tested all features and transitions as possible before merge this important and waiting feature :). Sorry for this small miss the following patch solve this problems on two points.

First isRequiredType() do first check if the element hasCropRequired() and then parse the element configuration but that's more a consequence and a small adjust of element more than real patch.

Secondary we need to adjust Widget miss because element need to have an empty array and not NULL by default that's is forced to null in default definition of widget we need to change that (and for crop_list too by the way does suffer than same problems).

Can you test it again please and switch to RTBC if all works well to you :)

Thanks.

Small fixe introduced conding standards issue sorry.

I can confirm, that the warnings are disappearing after applying the current patch. Thanks to all!

This patch is RTBC!

Merged Thanks all :) good JOB

merged into dev?
Stable does not seem to include this patch.
When patching DEV or Stable I get an error:

Fatal error: Cannot redeclare Drupal\image_widget_crop\Element\ImageCrop::isRequiredType() in /var/www/drupalvm/www/xxx/modules/contrib/image_widget_crop/src/Element/ImageCrop.php on line 280

I had the same problem with current versions of drupal core 8.7.4 and module conditional_field 8.x-1.0-alpha5. Only first imagefield with crop thrown an error if cropping was not applied but all where marked with a red required *. After disabling and enabling conditional_field. Resave form display settings, field settings field suddenly it worked.