The documentation for image_filter_keyword() doesn't describe the function parameters nor the value the function returns. Both should be added.

CommentFileSizeAuthor
#11 interdiff-5-11.txt795 bytesjungle
#11 3097797-11.patch1.37 KBjungle
#5 drupal-add-parameters-description-3097797-5.patch1.35 KBapaderno
#4 descriptions_for_parameter_and_the_returned_value-3097797-4.patch1.46 KBdev.patrick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kiamlaluno created an issue. See original summary.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

I would also change the name of the first parameter, as $value is a bit too vague.

dev.patrick’s picture

dev.patrick CreditAttribution: dev.patrick as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commented
Assigned: Unassigned » dev.patrick
dev.patrick’s picture

dev.patrick CreditAttribution: dev.patrick as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
descriptions_for_parameter_and_the_returned_value-3097797-4.patch1.46 KB

Adding patch to update description for parameters and return value.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
FileSize
drupal-add-parameters-description-3097797-5.patch1.35 KB

Given the code that calls that function, I would change the function description and add the parameters description as in the following patch.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

The method I am referring in my previous comment is CropImageEffect::applyEffect(), which contains the following code.

  list($x, $y) = explode('-', $this->configuration['anchor']);
  $x = image_filter_keyword($x, $image->getWidth(), $this->configuration['width']);
  $y = image_filter_keyword($y, $image->getHeight(), $this->configuration['height']);

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented
Issue tags: +Global2020

Tagging this issue for new contributors to work on during the Friday DrupalCon Global sprint.

ultrabob’s picture

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

The patch applied cleanly and I thoroughly tested The crop and scale and crop image effects that use this function. They all continue to work as expected.

I agree that the new parameter names are much easier to understand, and the docblock does a nice job of explaining what the function does. I don't think the function name image_filter_keywordis very intuitive, but I don't have a better idea either.

It is a good improvement, and is in good working order.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Yes, the function name would make me think it filters text to give a keyword. Without seeing code that uses that function, I would not have imagined its purpose.
I thought of changing the function name too, but I also thought to replace the function with a class method. The problem in this case is finding the right class; given it's called from two image effect plugins, replacing it with a ImageEffectBase's method could make sense, but it would not consider which third-party modules use that function. (Some of those modules would probably not expect to deal with the image effects base class.)

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer and at InterGreat Education Group commented
FileSize
3097797-11.patch1.37 KB
interdiff-5-11.txt795 bytes 
  1. +++ b/core/modules/image/image.module
@@ -341,26 +341,33 @@ function template_preprocess_image_style(&$variables) {
+ *   The anchor ('top', 'left', 'bottom', 'right', 'center'). ¶

    Redundant trail whitespace.

  2. +++ b/core/modules/image/image.module
@@ -341,26 +341,33 @@ function template_preprocess_image_style(&$variables) {
+  switch ($anchor) {
...
+      return $current_size / 2 - $new_size / 2;
...
-  return $value;
+  return $anchor;

    Could move to default in switch, for better readability

Addressing the above two points. Meanwhile, would be better adding unit tests for image_filter_keyword(), but not sure if it's in scope, so stay RTBC.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented

I double checked the changes in #11 and they seem ok to me.

Please note that @ultrabob in #9 was working with me and @mradcliffe during the DrupalCon Global contribution day. I do see now that there are no screenshots so let us know if those need to be added to move this forward. Thanks.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

@KristenPol Screenshots of code aren't much helpful.
Everybody can verify the patch contains trailing spaces that need to be removed. A screenshot could eventually be a further prove that what reported is correct, but the confirmation of other users (including the user who provided the patch) should be sufficient.

(I shall remember to set Sublime Text to show the line endings, before creating a new patch.)

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented

Right. Sorry, I was confusing this with a different issue that could be tested via the UI. Sorry for the noise.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

@KristenPol Asking what needs to be done to make a patch progress isn't noise at all. It's better to ask, when there are doubts.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Component: documentation » image.module
Status: Reviewed & tested by the community » Fixed
Issue tags: +Documentation

Committed 0eb1d66 and pushed to 9.1.x. Thanks!

  • alexpott committed 0eb1d66 on 9.1.x 
    Issue #3097797 by jungle, kiamlaluno, dev.patrick, Kristen Pol, ultrabob...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.