acquia / df

Demo Framework - mirrored at https://git.drupal.org/project/df.git
https://www.drupal.org/project/df
18 stars 19 forks source link

Focal point is 0,0 by default when using Unsplash Stream #174

Closed kevinfunk closed 4 years ago

kevinfunk commented 4 years ago

Trying to update to Focal Point 1.1, I've noticed that the focal point is set to 0,0 by default. If you open a media item and save it without setting the focal point, you will get the error Warning: Division by zero in Drupal\focal_point\FocalPointManager->absoluteToRelative() (line 64 of /focal_point/src/FocalPointManager.php) Screen Shot 2019-09-19 at 8 38 52 AM

saltednut commented 4 years ago

Was able to narrow this down to just edits. By default, nothing has a crop set up yet, so it should always be 50,50 when you first install.

After that, things can change. One thing we were doing during the presave setup here was using setValue() on the image field using an entire array. It works better to only update the ID and alt tags using the actual entity field object like $entity->image->alt = $entity->label();

Once we switched to this it fixed the issue where 0,0 was being set as the crop coordinates when using media/image/edit

saltednut commented 4 years ago

To be clear, this is the actual fix. I did do some other minor things (cleanup) in the commit.

https://github.com/acquia/df/commit/8083f5e55f2ec3ef8f528ab7af1382d8f7cb3c4f#diff-c9ac3e9464cf48375cf58f56c51ec931R158-R161

saltednut commented 4 years ago

Looks like we still have one edge case where newly created images via CDF import do not have any crop data even though they do have new crops added into the system. This is because they have the old crops in the CDF connected to them. We'll need to do a CDF re-export to fix this after manually fixing all the crops.

Images added via unsplash widget are not affected by any of this because they get the default crop OOTB via the UI.

One "fix" that can be applied is to keep the Unsplash value in place, but just re-save the image. This updates the image's crop value to be correct. So this should actually be fixed once we do that and re-export.

saltednut commented 4 years ago
Screen Shot 2019-09-20 at 2 34 29 PM
saltednut commented 4 years ago
Screen Shot 2019-09-20 at 2 33 19 PM
saltednut commented 4 years ago

Confirmed after a fresh install that all the default images have a crop set and its not 0,0 so the new CDF fix worked.

Screen Shot 2019-09-20 at 4 58 15 PM