Make WordPress Core

Opened 12 years ago

Closed 10 years ago

#22587 closed defect (bug) (wontfix)

Cast image sizes to array before looping

Reported by: griffinjt's profile griffinjt Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: Media Keywords: has-patch
Focuses: Cc:

Description

If someone doesn't filter the image_size_names_choose filter correctly, or uses some helper function like __return_false if they don't want to display any image sizes, a warning is thrown.

The $sizes var should be cast to an array before looping to avoid this warning.

Attachments (2)

22587.diff (460 bytes) - added by griffinjt 12 years ago.
22587.2.diff (438 bytes) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (18)

@griffinjt
12 years ago

#1 @scribu
12 years ago

  • Keywords close added

I don't think that's a good reason at all. You can just use __return_empty_array.

Last edited 12 years ago by scribu (previous) (diff)

#2 @JustinSainton
12 years ago

  • Keywords close removed

True, the reasoning may not be sound - but core has about 250+ instances of casting variables to an array in a foreach loop. +1 for parity's sake, not for bad dev use cases.

#3 follow-up: @scribu
12 years ago

So, your argument is "we should do it because we do it in a lot of other places". Not a very big improvement, is it?

#4 in reply to: ↑ 3 @MikeSchinkel
12 years ago

  • Cc mike@… added

Replying to scribu:

So, your argument is "we should do it because we do it in a lot of other places". Not a very big improvement, is it?

Why always so hostile? I wonder how many good developers have given up contributing on Trac because they get tired of people telling them their suggestions are stupid?

#5 follow-up: @griffinjt
12 years ago

What about those devs that don't know any better? Sure, we can educate them, but why not give a little protection in the process?

Not everyone is a good developer. I don't think this patch sacrifices anything significant to help them out.

#6 @JustinSainton
12 years ago

Replying to MikeSchinkel:

Replying to scribu:

So, your argument is "we should do it because we do it in a lot of other places". Not a very big improvement, is it?

Why always so hostile? I wonder how many good developers have given up contributing on Trac because they get tired of people telling them their suggestions are stupid?

I wouldn't necessarily consider his comments hostile. And he never called the suggestion stupid, to be fair. He just asked if my argument was that "we should do it because we do it in a lot of other places". And basically, that is my argument.

Scribu might think that's a dumb argument (I think consistency is actually a pretty good argument) - but he never said that. And it could be easily argued that there are three times as many instances that don't type-hint. I just happen to think it's a good practice.

#7 in reply to: ↑ 5 @scribu
12 years ago

Replying to griffinjt:

What about those devs that don't know any better? Sure, we can educate them, but why not give a little protection in the process?

Actually, this "protection" would

a) make devs lazy: ah, I can just pass __return_false, even if it should be an array.

b) make it harder to spot errors; for example, if you forget to return, you won't get an obvious error that you immediately see and can track down.

#8 follow-up: @nacin
12 years ago

MikeSchinkel: scribu has a point. If there's no good reason, there's no good reason. Justin was fine to suggest that there may be a parity argument, and scribu was fine to (in his usually colorful way) clarify that parity doesn't really work for such a general case. I imagine both of them would have continued to go back and forth until there was some kind of agreement, consensus, or at least understanding.

Instead, you stepped in to try to make a big deal about something/nothing. Since your comment, Justin returned and said he didn't have a problem with scribu's comment, the reporter returned and made a point, and scribu returned with two succinct points that I explain in much greater depth below. Three helpful contributions.


To the ticket. image_size_names_choose gets called in a few places — 3 now, I think, so that patch needs some more love.

There are two schools of thought for return values of filters:

  • Cast. It is better to make sure that a plugin doing it wrong does not mess up core execution.
  • Don't cast. A developer seeing an E_WARNING as a warning to fix their code is better.

These are not mutually exclusive. I think it depends on the situation for which makes the most sense. I am sure some casts in core should have been omitted, and indeed *some* could probably be removed (such as in cases where the variable is *always* an array). Most casts are probably in older code that just did it out of habit. Casting inside a foreach statement is one of two things, typically: a deliberate choice, or generic bad practice. In the end, it doesn't particularly matter. As long as we make common sense decisions for the two schools: when is a developer error is fine, and when does core really need to not run into an issue here?

The problem with casting false to an array is it does not end up with array(). <== This is important. You end up with array( 0 => false ). The only thing casting to an array helps with, is if the filter accidentally does not return anything, as in null, as casting null to an array does result in an empty array. Generally, casting makes more sense for an indexed array (just values) than it would for an associative array, as the key is likely important, while casting doesn't give you a key.

In this case, the issue isn't devs doing it wrong (very little evidence of that here). And given the previous few paragraphs, casting in this case doesn't make a lot of sense.

Last edited 12 years ago by nacin (previous) (diff)

#9 in reply to: ↑ 8 ; follow-up: @griffinjt
12 years ago

nacin - thanks for your detailed explanation. I really do appreciate you taking the time to educate me so I can hopefully contribute with better and more thought out patches in the future! :-)

I get the parity argument and I get scribu's argument as well - I'm not trying to pick a bone with anyone, trust me. I agree with both. It's not *that* big of a deal if we are to be honest. I just like going down all the rabbit holes to make sure that things are nearly unbreakable, and if that means protecting users and developers from themselves (which ultimately leads to less support), I'm all for it.

It's just a different thought process - that's all.

#10 in reply to: ↑ 9 @nacin
12 years ago

  • Version changed from trunk to 3.3

image_size_names_choose was introduced in 3.3.

Replying to griffinjt:

nacin - thanks for your detailed explanation. I really do appreciate you taking the time to educate me so I can hopefully contribute with better and more thought out patches in the future! :-)

I think this was a great patch and proposal, actually, even if I don't currently think it needs to go into core. I've thrown proposals against the wall many times to see if they stick. Sometimes, they are half-baked or stupid. Other times, someone else can find something good in them. Never hurts to propose something. I am simply here trying to walk through my current array-casting thought process.

I just like going down all the rabbit holes to make sure that things are nearly unbreakable

We like doing that too. Glad to see you on our side. :-)

#11 @griffinjt
12 years ago

Just looking further into this. In wp-admin/includes/media.php, this filter is again used. If you do use the correct helper function __return_empty_array, you are presented with a myriad of errors and issues. This is because $out is not defined beforehand and is assumed that it will be populated with image sizes. There should be a check to ensure $out is actually set and not empty before even creating the Size label and associated input fields.

Last edited 12 years ago by griffinjt (previous) (diff)

#12 @griffinjt
12 years ago

Sorry, forgot to mention that this is in the image_size_input_fields function in `wp-admin/includes/media.php'.

Version 0, edited 12 years ago by griffinjt (next)

#13 follow-up: @SergeyBiryukov
12 years ago

$out is defined in image_align_input_fields():
http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/media.php#L710

But not in image_size_input_fields(). Seems missed in [8612].

22587.2.diff fixes that.

#14 in reply to: ↑ 13 @griffinjt
12 years ago

Replying to SergeyBiryukov:

22587.2.diff fixes that.

Related to your patch: I believe this is more of a UX type of thing, but how should we handle outputting the HTML if no image sizes are present? It seems to me like the best course of action would be the simply hide the size label and field altogether. In that case, we could have the function either return false or an array, and then use a simple if check on the function before storing the data in the $form_field array key. Thoughts?

#15 @nacin
11 years ago

  • Component changed from Warnings/Notices to Media

How we used the results of this filter was significantly changed in 3.5; wondering if there are any other adjustments to make here.

#16 @wonderboymusic
10 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

There are 4 main uses of this in core now, 2 others that are deprecated. Casting doesn't make any sense, and I can't think of a great use case for someone blanking out every image size. A PHP warning goes a long way here - telling the dev that their code or some plugin is essentially breaking the media experience.

Note: See TracTickets for help on using tickets.