Depend on Crop API for storage

Looks good

1. +++ b/focal_point.module
   @@ -23,12 +23,42 @@ function focal_point_entity_presave(EntityInterface $entity) {
   +            // Focal point JS provides relative location while crop entity
   +            // expects exact coordinate on the original image. Let's convert.
   +            $x = (int) round(($x / 100.0) * $item->width);
   +            $y = (int) round(($y / 100.0) * $item->height);
   

   This was my biggest question when you first proposed using cropAPI... I see how this will work now. ++

2. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -93,8 +94,12 @@ class FocalPointImageWidget extends ImageWidget {
   +      $crop = Crop::findCrop($file->getFileUri(), \Drupal::config('focal_point.settings')->get('crop_type'));
   

   crop could be NULL here, no?

3. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -93,8 +94,12 @@ class FocalPointImageWidget extends ImageWidget {
   +      $x = (int) round($crop->x->value / (float) $return['width'] * 100);
   +      $y = (int) round($crop->y->value / (float) $return['height'] * 100);
   

   why are you typecasting the width and height to floats here?

4. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -108,8 +113,8 @@ class FocalPointImageWidget extends ImageWidget {
   +    if (!is_null($focal_point_value) && !preg_match('/^(100|[0-9]{1,2})(,)(100|[0-9]{1,2})$/', $focal_point_value)) {
   

   I hate having a regex like this here in a random if ... can we create a validate method somewhere no that it's gone?

5. +++ b/src/Plugin/ImageEffect/FocalPointCropImageEffect.php
   @@ -27,11 +27,33 @@ class FocalPointCropImageEffect extends FocalPointEffectBase {
   +        'Focal point scale and crop failed while scaling and cropping using the %toolkit toolkit on %path (%mimetype, %dimensions)',
   +        [
   +          '%toolkit' => $image->getToolkitId(),
   +          '%path' => $image->getSource(),
   +          '%mimetype' => $image->getMimeType(),
   +          '%dimensions' => $image->getWidth() . 'x' . $image->getHeight()
   +        ]
   

   probably worth including the anchor here as well

6. +++ b/src/Plugin/ImageEffect/FocalPointScaleAndCropImageEffect.php
   @@ -30,15 +30,46 @@ class FocalPointScaleAndCropImageEffect extends FocalPointEffectBase {
   +        'Focal point scale and crop failed while resizing using the %toolkit toolkit on %path (%mimetype, %dimensions)',
   +        [
   +          '%toolkit' => $image->getToolkitId(),
   +          '%path' => $image->getSource(),
   +          '%mimetype' => $image->getMimeType(),
   +          '%dimensions' => $image->getWidth() . 'x' . $image->getHeight()
   +        ]
   

   ... anchor info. Maybe worth having a helper method since we have this at least twice

+++ b/focal_point.module
@@ -23,12 +23,42 @@ function focal_point_entity_presave(EntityInterface $entity) {
+            if (Crop::cropExists($item->entity->getFileUri(), $crop_type)) {

This method doesn't exist in 8.x-alpha1... are you developing off of a newer version of CropAPI? In any event, it looks like we would need to either fix this patch to no longer use cropExists or to set a minimum value on the dependency. Assuming the later, I wouldn't want to merge this until there is another tagged release of Crop API.

I also had another question that I meant to bring up in my first review but forgot. Currently there is a feature in the 7.x version of focal point that allows a site admin to specify a shift value (x & y) for each image preset. (see https://www.drupal.org/node/2386981#comment-9413383 for use cases). I hadnt yet ported this to the 8.x version but I'm sure I will at some point... With this change, I dont see why I couldnt just save those shift values with the image preset (like I do now in 7.x) and then apply the shift to the crop object once its been loaded (but before the effects are applied) but I wanted to throw it out there to see if you foresee any problems with this that I'm not thinking of

1. +++ b/focal_point.info.yml
   @@ -6,3 +6,4 @@ package: Images
   +  - crop
   

   shouldn't this now be crop (8.x-alpha2) or whatever the equivilant in D8 is? I havent actually had to do a version specific dependency in D8 yet

2. +++ b/focal_point.install
   @@ -5,37 +5,82 @@
   +  if ($phase == 'update' && !\Drupal::moduleHandler()->moduleExists('crop')) {
   

   same with this ... can we check for a version?

... otherwise I think this is pretty close.

I ran this by one of my co-workers (@ericduran) and he gave me a bunch of good feedback. Below is one item he brought up and I'm bothering him to put more feedback directly in this issue...

+++ b/focal_point.module
@@ -23,12 +23,42 @@ function focal_point_entity_presave(EntityInterface $entity) {
+            list($x, $y) = explode(',', $item->focal_point);
+
+            // Focal point JS provides relative location while crop entity
+            // expects exact coordinate on the original image. Let's convert.
+            $x = (int) round(($x / 100.0) * $item->width);
+            $y = (int) round(($y / 100.0) * $item->height);

Having this in a presave hook is not a great idea. There is no good way to (for example) save a focal point value programatically if we do this. We probably need to reinstate the FocalPoint class to handle some of this...

...havent forgotten about this. Swamped. Stay tuned

I couldn't run the update on my local machine, because there are a lot of files missing. Normally they get downloaded by stage file proxy. So the getimagesize is breaking. Maybe we could retrieve the size from the fields where the images are used. Because there are the sizes saved.

Sorry I took so long to get back to this but it has been crazy busy. For real though, this is really really close. Thanks for workin hard on this

1. +++ b/src/FocalPointManager.php
   --- /dev/null
   +++ b/src/FocalPointManagerInterface.php
   

   I like this approach so MUCH better!!! Thanks!

2. +++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
   @@ -93,8 +96,17 @@ class FocalPointImageWidget extends ImageWidget {
   +        $return['focal_point'] = "{$coords['x']},${coords['y']}";
   

   nit: lets be consistent with the "{" coming either before or after the "$"

3. +++ b/src/Plugin/ImageEffect/FocalPointCropImageEffect.php
   @@ -27,11 +27,34 @@ class FocalPointCropImageEffect extends FocalPointEffectBase {
   +        'x' => (int) round($image->getWidth() / 2),
   +        'y' => (int) round($image->getHeight() /2),
   
   +++ b/src/Plugin/ImageEffect/FocalPointScaleAndCropImageEffect.php
   @@ -30,15 +30,47 @@ class FocalPointScaleAndCropImageEffect extends FocalPointEffectBase {
   +        'x' => (int) round($image->getWidth() / 2),
   +        'y' => (int) round($image->getHeight() /2),
   

   Edge case, but if someone changes the default value (50,50) config to something other than 50,50, than this is no longer correct.

4. +++ b/src/Plugin/ImageEffect/FocalPointScaleAndCropImageEffect.php
   @@ -30,15 +30,47 @@ class FocalPointScaleAndCropImageEffect extends FocalPointEffectBase {
   +    $crop_type = $this->focalPointConfig->get('crop_type');
   +    /** @var \Drupal\crop\CropInterface $crop */
   +    if ($crop = Crop::findCrop($image->getSource(), $crop_type)) {
   +      $crop->width = $this->configuration['width'];
   +      $crop->height = $this->configuration['height'];
   +    }
   +    else {
   +      $crop = $this->cropStorage->create([
   +        'type' => $crop_type,
   +        'x' => (int) round($image->getWidth() / 2),
   +        'y' => (int) round($image->getHeight() /2),
   +        'width' => $this->configuration['width'],
   +        'height' => $this->configuration['height'],
   +      ]);
   +    }
   +
   +    $anchor = $this->calculateAnchor($image, $crop);
   +    if (!$image->crop($anchor['x'], $anchor['y'], $this->configuration['width'], $this->configuration['height'])) {
   +      $this->logger->error(
   +        'Focal point scale and crop failed while scaling and cropping using the %toolkit toolkit on %path (%mimetype, %dimensions, anchor: %anchor).',
   +        [
   +          '%toolkit' => $image->getToolkitId(),
   +          '%path' => $image->getSource(),
   +          '%mimetype' => $image->getMimeType(),
   +          '%dimensions' => $image->getWidth() . 'x' . $image->getHeight(),
   +          '%anchor' => $anchor,
   +        ]
   +      );
   

   Rather than duplicate this entire chunk, can we just create a CalculateCrop method in the focalPoint service that both effects could call?

+++ b/src/Plugin/Field/FieldWidget/FocalPointImageWidget.php
@@ -7,7 +7,10 @@
+use Drupal\Core\StringTranslation\ranslationTrait;

This should be Drupal\Core\StringTranslation\TranslationTrait; but i dont see where it is being used

This patch is meant to

• chase HEAD. The patch in #25 won't apply
• address #27.2
• address #27.4 (by adding a applyCrop method to the focalPointEffectBase class)
• address #30 (though I still dont know why we are using that trait)

Sorry there is no interdiff; I couldn't apply the previous patch so I couldn't create one.

On a different note: when I try to run the PHPUnit tests I get this 11 times:

1) Drupal\Tests\focal_point\Unit\Effects\FocalPointEffectsTest::testCalculateResizeData with data set #0 (640, 480, 300, 100, array(300, 225))
Exception: There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable to run PHPUnit based functional tests outside of run-tests.sh. See https://www.drupal.org/node/2116263#skipped-tests for more information.

I don't understand this error. Why would unit tests be looking for a DB connection?

The patch in #31 is missing new files.

Okay ... I think this might be it.

The previous patches had issues with the calculateAnchor method (it was not taking into account the entire crop area; it was simply making sure the anchor point fell within the image. In addition, this patch improves the unit tests and it fixed a few minor errors.

Can I get a review?

Let's see if this satisfies testbot... the key change that I think will fix it is this:

-    $crop = $this->getMockBuilder('Drupal\crop\Entity\Crop')->disableOriginalConstructor()->getMock();
+    $crop = $this->getMockBuilder('Drupal\crop\CropInterface')->disableOriginalConstructor()->getMock();

The other changes are meant to be general cleanups .. nothing major

Ah, the testbot cannot figure out that it needs to download crop first.
You have two options.
Make a commit first that just adds the dependency, push that, wait 24 hours, then retest this.
Or if you're uncomfortable with that, you can add it as test_dependencies the same way, and then swap it to regular dependencies when committing this.

Also I'm going to do a quick pass rewriting these unit tests to prophecy-based mocks, just to show you an alternative :)

/me shakes fist at dependencies!!

Thanks @tim.plunkett ... I'm going to add to test_dependencies now...

Also I'm going to do a quick pass rewriting these unit tests to prophecy-based mocks, just to show you an alternative :)

That would be awesome!! I'm still figuring out PHPUnit tests :)

Thanks again

Here is a bit of unit test rejiggering. Note the changes to calculateResizeDataProvider, you can pass arbitrary strings as the array keys, and they'll show up if that single data provider fails, which is super nice. I'd recommend you do the same for calculateAnchorProvider, to explain which cases are which.

rerolling #42

(patch didnt apply becuase of the test_dependencies I added)

I'd recommend you do the same for calculateAnchorProvider, to explain which cases are which.

Also did this fanciness

Converting FocalPointTest.php to prophesize based unit tests as well.

A few small cleanups

So in (finally) getting the unit tests in order and other cleanups as well, I managed to break the actual cropping functionality in Focal Point.

This fixes the cropping functionality

Fatal error: Unsupported operand types in install.core.inc on line 1016

- missing return value of hook_requirements
- some code style fixes

I don't particularly care myself, but why was I removed from the commit message? The generated one at the bottom includes me.

@tim.plunkett ... I'll be completely honest. I still don't understand the etiquette of the commit messages. Someone (and I cant remember who) explained to me that its supposed to be the major "meat" of the patch and that it specifically shouldn't be everyone involved in an issue. I'm guessing, based on your comment, that this is not exactly correct? 99% of issues I work on have (at most) me and one other person working on it.

Please don't think that I don't appreciate your help. I really am thankful that you jumped in (especially at such a frustrating moment).

... guidance welcome

I dont get it ... all tests were passing when I committed this and now they are not. This patch is a rollback of the commit of this issue. I'm just trying to debug the D.O. unit test failures.

Lets see what happens

lets try that again

Blarg ... wrong diff. One more try

testbot ignores hidden files...sigh

This patch make s a text change to README

Setting back to fixed ... I think I have enough info to (at least) rule out that testbot is just drunk. Not sure how these tests *used* to pass though and why it is that they still pass locally.

/me shakes fist

I came to the same conclusion (with help from tim.plunkett on irc) about 30 mins ago: http://cgit.drupalcode.org/focal_point/commit/?id=1fb7d54

Ok ... phew ... looks like this worked.

@slashrsm would you take a look at #2733425: Scale and crop effect not scaling and cropping as expected. I really want to get that in quickly since the math is technically broken right now