WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 3 months ago

#42463 assigned defect (bug)

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:

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 (5)

42463.diff (1009 bytes) - added by nikolastoqnow 21 months ago.
Add better description about crop parameter in add_image_size function
42463.patch (974 bytes) - added by ketuchetan 20 months ago.
42463.2.patch (971 bytes) - added by ketuchetan 20 months ago.
42463.3.diff (1.0 KB) - added by audrasjb 3 months ago.
42463 Docs patch refresh
42463.4.diff (1.1 KB) - added by killua99 3 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.

Download all attachments as: .zip

Change History (24)

@nikolastoqnow
21 months ago

Add better description about crop parameter in add_image_size function

#1 @nikolastoqnow
21 months ago

  • Keywords has-patch added; needs-patch removed

#2 @ocean90
21 months 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
20 months ago

#3 @ketuchetan
20 months 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

#4 @miqrogroove
20 months 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.


18 months ago

#6 @mikeschroder
10 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
10 months ago

  • Keywords reporter-feedback added

#8 @miqrogroove
10 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
10 months ago

  • Keywords reporter-feedback removed

#10 @peterwilsoncc
9 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
6 months ago

  • Milestone changed from 5.1 to 5.2

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


4 months ago

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


4 months ago

#14 @audrasjb
3 months ago

  • Keywords needs-refresh added

The patch doesn't applies cleanly.

@audrasjb
3 months ago

42463 Docs patch refresh

#15 @audrasjb
3 months ago

  • Keywords needs-refresh removed

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

#16 @joemcgill
3 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
3 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.


3 months ago

#18 @joemcgill
3 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
3 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.

Note: See TracTickets for help on using tickets.