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.

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.

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.

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

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.

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?

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.