[D8] ImageWidgetCrop

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxwoprrr2571403git

Fixed the git clone URL in the issue summary for non-maintainer users.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Fixed the git clone URL in the issue summary for non-maintainer users.

I'm a robot and this is an automated message from Project Applications Scraper.

Hi woprrr

I focused my first code review in your hook_entity_presave implementation.

1. Image widget service

Why are you call the image_widget_crop.manager service in each iteration of the foreach ?

/** @var \Drupal\image_widget_crop\ImageWidgetCrop $crop */
$crop = \Drupal::service('image_widget_crop.manager');

It would be better to move this piece of code outside the foreach.

2. Route parameter

Why you check the 'edit' value in route parameter ?

// Get the route params attributes.
$route_params = \Drupal::requestStack()
->getCurrentRequest()->attributes->get('_route_params');

if (isset($route_params['_entity_form']) && preg_match('/.edit/', $route_params['_entity_form'])) {
  $edit = TRUE;
}

If I understand, you would know if the current saving request comes from an edit form, is that right ?
You can use $entity->isNew() to check that.

node/add : $entity->isNew() == TRUE
node/#/edit : $entity->isNew() == FALSE

3. Multiple value

Your module works fine with contents containing an image field with a number of value equals to 1.
I tried to add a field with multiple value, Only the first crop is saved.
In this hook_entity_presave, this code is the cause

$entity_fields->getValue()['0']

You must iterate on all field value.

You must handle these cases :

New entity -> No images
New entity -> Add 1 image
New entity -> Add 2 (or more) images
Edit entity -> No images -> Add 1 image
Edit entity -> No images -> Add 2 (or more) image
Edit entity -> 1 image -> Add more images
Edit entity -> 2 (or more) images -> Add more images
Edit entity -> 1 image -> Remove 1 image
Edit entity -> 2 (or more) images -> Remove 1 image
Edit entity -> 2 (or more) images -> Remove 2 (or more) images
Delete entity -> delete all images

Nice !
I watched the rest of your code. Everything seems good !

Just one thing, I found a bug by following this steps :

1. Allow multiple value in your field
2. Create a new content
3. Add one image
4. Select the crop zone
5. Add another image

Paf, previously selected crop zones are disapeared !

Maybe javascript ?

Hi woprrr,

please fix the errors in parreview

Manual Review:
Individual user account: Follows
No duplication: See #4
Master Branch: Follows
Licensing: Follows
3rd party assets/code: N/A
README.txt/README.md: Follows
Coding style & Drupal API usage:
Automated review: errors

Removing review bonus tag, you have not listed any reviews in the issue summary? Make sure to read through the source code of the other projects, as requested on the review bonus page.

Review of the 8.x-1.x branch (commit 2b27b12):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• ESLint has found some issues with your code (please check the JavaScript coding standards).
  

  /home/klausi/pareview_temp/js/imageWidgetCrop.js: line 97, col 27, Error - eval can be harmful. (no-eval)
  
  1 problem
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. project page: what is the use case of the module, what problem does it solve? Does it operate on image fields I assume? Or on Form API file elements? What image toolkit does it use, how does it crop the images? What does the UI look like, can you provide a screenshot on the project page?
2. image_widget_crop.info.yml: why does the module depend on the node module? Please add a comment.
3. image_widget_crop.services.yml: the entity manager service is deprecated. use one of the replacements instead, see https://www.drupal.org/node/2549139
4. "method_exists($entity, 'getFields')": do not check if the method exists, check if the entity implements the correct interface with instanceof, FieldableEntityInterface.
5. ImageWidgetCropManager: a service should usually not invoke drupal_set_message(), since it might be used in other contexts where no messages should be shown. Use return values instead to indicate success/failure. Or use your notify parameter pattern everywhere.
6. ImageWidgetCropManager: do not use \Drupal in service classes. Inject the service instead. Also elsewhere in ImageCropWidget.php for example.
7. getCropOriginalDimension: RuntimeException: what image file is not valid? Can you pass on an ID or something?
8. "@return array" should be "@return double[]"
9. ImageCropWidget::process: why do you need the route params here? why do you need to check if you are on a form here? Please add a comment.
10. " Add compatibility to PHP 5.3.": Drupal 8 core depends on PHP 5.5, so you should remove all PHP 5.3 compatibility code and comments.
11. ImageCropWidget: don't use the function t() in objects if possible. Use $this->t() instead in this case, which is already on the parent class.
12. ImageWidgetCropTest: don't use \Drupal here, use $this->container->get() to access services.
13. ImageWidgetCropTest: $this->randomMachineName(): do not use random test data in tests which makes them unpredictable. See https://www.drupal.org/node/2571183

But that are not critical application blockers, otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to Dave Reid as he might have time to take a final look at this.

In ImageWidgetCropManager we have any /Drupal call, in ImageCropWidget class is obligatory in static méthod. process method is defined static in core we can change it to inject our code. In github we have an issue to transform all element define in process into new form element ( @see https://github.com/drupal-media/image_widget_crop/pull/1#issuecomment-17... ).

One \Drupal::service() call is in static function, but the other is not. Also, deprecated entity.manager is still used in one of them. Would be great to check entire module for usages of entity manager.

It's same, i process scrutinizer analyze and he require an @return array instead double[].

Drupal standard is double[].

We need to be sure to be in an editing context of an entity when we access a process().

I agree with @klausi that this isn't the nicest solutions. Why can't we simply try to load? If nothing comes back it is obvious that there is no crop for the image. But I also agree that it is not critical. Specially considering the fact that we're planning to refactor this part significantly. As @woprrr already mentioned we're planning to convert cropping tool to a decoupled form element. We will fix this kind of problems along the way.

Unrelated to stuff above.... This module has already been adopted by Media initiative. It has a copy in drupal-media group on Github and it following our standard development process. We invested significant amount of time into it in past few months. It was one of the focuses at Zurich media sprint, that happened in December.

It was great to work with @woprrr. He is very open for other ideas, very collaborative, responsive and open for suggestions. All this will make him a great module maintainer.

This is good to go as far as I am concerned. I am pretty sure everyone in the initiative that worked on this module (many people!) agree with me.

OK, I think another RTBC from @slashrsm is enough to approve this.

Thanks for your contribution, woprrr!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.