Cast image sizes to array before looping

[griffinjt]

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.

[scribu]

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

[JustinSainton]

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.

[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?

[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?

[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?

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

[JustinSainton]

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.

[scribu]

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.

[nacin]

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.

[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 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.

[nacin]

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. :-)

[griffinjt]

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.

[griffinjt]

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

[SergeyBiryukov]

$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.

[griffinjt]

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?

[nacin]

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

[wonderboymusic]

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.