Problem/Motivation

The Drupal core security update 7.88 changed handling of Form API fields of type "value." After applying the core update, it is no longer possible to remove or change an existing crop.

Steps to reproduce

  1. Download and install the latest version of Panopoly 7.x (includes Manual Crop). Install demo content.
  2. Update to Drupal Core 7.88 (may already be updated depending on the timing of Panopoly's release).
  3. Upload a new image file at /file/add.
  4. Save the file without any cropping.
  5. Click the link to the new file in the Files table. Note that the entire image is displayed without any cropping.
  6. Click the Edit tab for the file. Note that the thumbnail shows the entire file without any cropping.
  7. Using the Crop button, crop the image. Select a portion of the image and Click Save on the crop screen. Note the designation "cropped" has been added to the Crop button. Note that the thumbnail shows the cropped version of the image.
  8. Click Save on the managed file edit page.
  9. Note that the image is displayed cropped.
  10. Click the Edit tab for the file again. Note that the designation "cropped" still appears on the Crop button. Using the Crop button, remove the selection and click Save on the crop screen. Note that "cropped" has been removed from the Crop button. Note that the thumbnail shows the cropped version.
  11. Click Save on the managed file edit page.
  12. Note that the image is still displayed cropped.
  13. Edit the file again. Note that the designation "cropped" still appears on the Crop button. Using the Crop button, maximize the selection and click Save on the crop screen. Note that the thumbnail shows the maximized version.
  14. Click Save on the managed file edit page.
  15. Note that the image is still displayed with its original non-maximized crop.

There were no errors in the JS console or the dblog.

Proposed resolution

There may be something that needs to be updated with generating or processing the form.

Remaining tasks

Patch and test

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#6 manualcrop-d788-3264865-alt.patch687 bytescurlymike
#2 manualcrop-d788-3264865-2.patch561 bytesdsnopek

Comments

cboyden created an issue. See original summary.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs review
Issue tags: +panopoly
StatusFileSize
new manualcrop-d788-3264865-2.patch561 bytes

The attached patch fixes the problem in my testing.

The change in 7.88 prevents elements of type 'value' from getting processed. However, in this case, this isn't really even a 'value' element, its a parent of a 'hidden' element, which is what is ultimately processed and contains the crop information. So, I think it's safe to remove the '#type' entirely, although, maybe it could be made a 'container' instead?

Assuming this works in testing by others, I'll get it integrated into Panopoly for the next release.

cboyden’s picture

cboyden commented

I've tested this on a Panopoly child distribution and it works to fix the issue without any side effects that I can see.

stephen-cox’s picture

stephen-cox commented

Patch #2 works for me. After upgrading Manual Crop to 1.7 and applying the patch, image cropping works again with the latest Drupal 7.

stephen-cox’s picture

stephen-cox commented
Status: Needs review » Reviewed & tested by the community
curlymike’s picture

curlymike commented
StatusFileSize
new manualcrop-d788-3264865-alt.patch687 bytes

Hi, everyone!
I'm using manualcrop on a custom form (aside FAPI image fields) and have been affected by this issue.
Removing element #type does not sit well with me, so I looked deeper into the code and I do not understand why $element['manualcrop']['file_' . $file->fid]['#default_value'] (element #type value) array created in manualcrop_croptool_process() (7.x-1.7 manualcrop.helpers.inc line 218,247) overlaps it's child elements $element['manualcrop_selections'][$style_name] (#type hidden) added by _manualcrop_add_croptool() (line 440). It even picks values for hidden elements from parent's values array! Those values going to $form_state['values'] definitely overlap and value of the #type => value parent overwrites values of it's #type => hidden children it seems.
Here is the patch that solves the issue for me, it doesn't seem to break FAPI usage or anything else (for me), but it would be great if someone who's familiar with manualcrop inner workings looked into it.
For now I'm using a workaround and set $element['manualcrop']['#default_value'] = array() for all added elements after calling manualcrop_croptool_process(), this way I don't need to patch manualcrop.

curlymike’s picture

curlymike commented
Status: Reviewed & tested by the community » Needs review