Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#53540 closed defect (bug) (fixed)

Use $image[0] instead of $image['0'] in media file

Reported by: chintan1896's profile chintan1896 Owned by: audrasjb's profile audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch good-first-bug
Focuses: Cc:

Description

I think we need to use $image[0] instead of $image['0'] in media file.
When called for srcset that time we have used $image[0] Please check here. https://github.com/WordPress/WordPress/blob/master/wp-includes/media.php#L1218

Attachments (2)

53540.diff (537 bytes) - added by chintan1896 3 years ago.
53540.1.diff (1.9 KB) - added by costdev 2 years ago.
Patch updated to cover the 6 remaining occurrences in core.

Download all attachments as: .zip

Change History (17)

@chintan1896
3 years ago

#1 @desrosj
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#2 follow-up: @adamsilverstein
3 years ago

@chintan1896 thanks for the patch. I'm curious why you noticed this issue, did it create a warning or error in your system? Or was it more something you noticed when reviewing the source code?

When using arrays, PHP will automatically cast the '0' string to 0 int, so this is more of a cosmetic fix than functional, is that right?

#3 in reply to: ↑ 2 @chintan1896
3 years ago

Replying to adamsilverstein:

@chintan1896 thanks for the patch. I'm curious why you noticed this issue, did it create a warning or error in your system? Or was it more something you noticed when reviewing the source code?

When using arrays, PHP will automatically cast the '0' string to 0 int, so this is more of a cosmetic fix than functional, is that right?

I have noticed when reviewing the source code. Yes, you are right this is a cosmetic fix. I have suggested for code consistency. We have used 0 in https://github.com/WordPress/WordPress/blob/master/wp-includes/media.php#L1218

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


3 years ago

#5 @antpb
3 years ago

  • Keywords good-first-bug added

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


2 years ago

#7 @antpb
2 years ago

  • Owner set to antpb
  • Status changed from new to assigned

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


2 years ago

#9 @costdev
2 years ago

Here's a link to confirm that this change will still work: https://3v4l.org/L7aNL

This change is worth making for the following reasons:

  • Readability - When scanning code, single- or double-quoted array keys will read as associative arrays at first glance. This change makes it immediately clear that this is a numeric array.
  • Performance - See the "Time with string|int" results in the output of the link above.
  • Consistency - Numeric keys should be written as integers.
  • Cross-version PHP support - As shown in the link above, this change will work across PHP versions.


IMO, this patch is ready for commit in time for Beta 1 today.


Any other occurrences of numeric string keys in core should be removed in a follow-up ticket.

A regex search for \[( )?'[0-9]'( )?\] returns the following additional occurrences:

Core:

  • src/wp-admin/includes/media.php (6)

These occurrences can have the same change made.

Upstream:

  • getID3 - src/wp-includes/ID3/module.audio.mp3.php (3)
  • @wordpress/components - src/wp-includes/js/dist/components.js (1)
  • @wordpress/components - src/wp-includes/js/dist/components.js.map (1)
  • @wordpress/editor - src/wp-includes/js/dist/editor.js (1)
  • @wordpress/editor - src/wp-includes/js/dist/editor.js.map (1)

These occurrences are an upstream matter.

Searches for more than single digit numeric keys only shows ['01'], ['02']... which refer to months. This would probably have been better implemented as a numeric array with the leading zeroes added when needed, but any changes would require investigation for potential BC and performance issues.

Last edited 2 years ago by costdev (previous) (diff)

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


2 years ago

@costdev
2 years ago

Patch updated to cover the 6 remaining occurrences in core.

#12 @audrasjb
2 years ago

  • Owner changed from antpb to audrasjb

Self-assigning for final review.

#13 @audrasjb
2 years ago

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

In 52245:

Media: Replace some array keys with their numeric equivalent.

This change replaces ['0'] with [0] which brings better consistency, readability and performance.

Props chintan1896, adamsilverstein, costdev.
Fixes #53540.

#14 @audrasjb
2 years ago

Thanks @costdev for your detailed tests and feedback 👍

Note: See TracTickets for help on using tickets.