WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 6 weeks ago

#42463 closed defect (bug) (fixed)

Poor Description of add_image_size Params

Reported by: miqrogroove Owned by: nikolastoqnow
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.9
Component: Media Keywords: has-patch
Focuses: docs Cc:
PR Number:

Description

From https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/media.php#L274

274	 * @param bool|array $crop   Optional. Whether to crop images to specified width and height or resize.
275	 *                           An array can specify positioning of the crop area. Default false.

So it literally says to do one thing or another. But which is what? "Default false" has a meaning, but it is totally lost in this documentation.

Attachments (8)

42463.diff (1009 bytes) - added by nikolastoqnow 2 years ago.
Add better description about crop parameter in add_image_size function
42463.patch (974 bytes) - added by ketuchetan 2 years ago.
42463.2.patch (971 bytes) - added by ketuchetan 2 years ago.
42463.3.diff (1.0 KB) - added by audrasjb 7 months ago.
42463 Docs patch refresh
42463.4.diff (1.1 KB) - added by killua99 7 months ago.
According with the discussion on slack channel the wording wasn't accurate and the need of mention about the extended description on the summary could help to describe the $crop argument.
42463.5.patch (2.0 KB) - added by pierlo 2 months ago.
42463.2.diff (2.0 KB) - added by joemcgill 6 weeks ago.
42463.5.diff (2.1 KB) - added by joemcgill 6 weeks ago.

Download all attachments as: .zip

Change History (36)

@nikolastoqnow
2 years ago

Add better description about crop parameter in add_image_size function

#1 @nikolastoqnow
2 years ago

  • Keywords has-patch added; needs-patch removed

#2 @ocean90
2 years ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to nikolastoqnow
  • Status changed from new to assigned
  • Version changed from trunk to 3.9

Introduced in [27472].

@nikolastoqnow Thanks for your patch! Looks like an empty line slipped into the patch. The use of "we" should also be avoided.

@ketuchetan
2 years ago

#3 @ketuchetan
2 years ago

  • Keywords good-first-bug needs-refresh removed

Hi @ocean90 ,

I have made the changes as you requested in @nikolastoqnow patch.

Please let me know if any changes on that.

Thanks,
Chetan

@ketuchetan
2 years ago

#4 @miqrogroove
2 years ago

How about adding something to the effect of:

  • Resize to fit one dimension, then crop image to fit the other dimension.
  • Keep source aspect and resize to fit both dimensions, allowing one dimension to become smaller than specified.

This is roughly how I would describe the two different outcomes. And assign them to "true" and "false".

This ticket was mentioned in Slack in #core-media by mike. View the logs.


22 months ago

#6 @mikeschroder
14 months ago

Taking a look at this, and noticed that there is an existing extended description further up in the docs that was added in [27472].

Is this sufficient? Should we move some of that documentation down, or add to it?

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/media.php#L253

#7 @mikeschroder
14 months ago

  • Keywords reporter-feedback added

#8 @miqrogroove
14 months ago

The docblock is totally inadequate. My reading of it is literally "We will either scale or crop your photo, and the default is false." So wth? Am I to understand that if I set "true", then a 1000 x 1000 image will be "cropped" to, say, the middle 100x100 pixels and never resized? Based on my experience, that is not even accurate.

#9 @miqrogroove
14 months ago

  • Keywords reporter-feedback removed

#10 @peterwilsoncc
13 months ago

  • Milestone changed from 5.0 to 5.1

Moving to the 5.1 milestone due to the WordPress 5.0 focus on the new
editor (Gutenberg).

#11 @pento
10 months ago

  • Milestone changed from 5.1 to 5.2

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


7 months ago

#14 @audrasjb
7 months ago

  • Keywords needs-refresh added

The patch doesn't applies cleanly.

@audrasjb
7 months ago

42463 Docs patch refresh

#15 @audrasjb
7 months ago

  • Keywords needs-refresh removed

I don't checked the wording itself but it is now refreshed against current trunk.

#16 @joemcgill
7 months ago

  • Keywords needs-patch added; has-patch removed

Thanks for the refresh @audrasjb. The latest change to the wording improves the understanding of the boolean value, but omits the behavior of the $crop parameter if it's set as an array. Ideally, the description for this parameter would summarize the behavior outlined in the description for this function:

If false (default), images will be scaled, not cropped.
If an array in the form of array( x_crop_position, y_crop_position ):
x_crop_position accepts ‘left’ ‘center’, or ‘right’.
y_crop_position accepts ‘top’, ‘center’, or ‘bottom’. Images will be cropped to the specified dimensions within the defined crop area.
If true, images will be cropped to the specified dimensions using center positions.

@killua99
7 months ago

According with the discussion on slack channel the wording wasn't accurate and the need of mention about the extended description on the summary could help to describe the $crop argument.

This ticket was mentioned in Slack in #core-media by killua99. View the logs.


7 months ago

#18 @joemcgill
7 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 5.2 to 5.3

Thanks for the update, @killua99. I think we can still improve this slightly before committing. However, with 5.3 RC1 so close, I'm going to go ahead and move this forward to the 5.3 milestone and we can continue to iterate.

#19 @killua99
7 months ago

I feel the same @joemcgill with the 5.2 RC 1 so close better to move it to 5.3. Let me know what do you have on your mind about grammar improvement.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


2 months ago

@pierlo
2 months ago

#21 @pierlo
2 months ago

In the patch 42463.5.patch I've moved the description of the cropping behavior to the $crop param doc.

This should resolve all ambiguity with the current explanation of the $crop param, as it feels silly to re-explain the description of it provided above.

Last edited 2 months ago by pierlo (previous) (diff)

This ticket was mentioned in Slack in #core-media by pierlo. View the logs.


8 weeks ago

This ticket was mentioned in Slack in #core-media by pierlo. View the logs.


7 weeks ago

#24 @mikeschroder
6 weeks ago

  • Keywords dev-feedback added

@joemcgill Do you want to do anything specific here, or should I make a couple final adjustments and commit?

Moving the docs to the param should allow IDEs to show the details as expected, correct?

This ticket was mentioned in Slack in #core-media by mike. View the logs.


6 weeks ago

@joemcgill
6 weeks ago

#26 @joemcgill
6 weeks ago

  • Keywords dev-feedback removed

I think the approach @pierlo is taking in 42463.5.patch is really close. I've cleaned it up a bit in 42463.2.diff and think it's ready to commit.

@joemcgill
6 weeks ago

#27 @joemcgill
6 weeks ago

42463.5.diff just cleans up some whitespacing but is otherwise identical to 42463.2.diff. Aside: auto-numbering works a lot better when we use diff as the extension since that's what grunt upload_patch uses.

#28 @joemcgill
6 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 46376:

Media: Improve documentation for add_image_size()

This improves the description of the $crop parameter to clarify behavior.

Props nikolastoqnow, ketuchetan, audrasjb, killua99, pierlo.
Fixes #42463.

Note: See TracTickets for help on using tickets.