[theme-reviewers] Theme quality review process

[Chip Bennett]

Adding an isset() conditional to checkbox option handling is entirely
appropriate, and, as far as I know, perfectly acceptable.

If I were to suggest any rewriting of the options handling, it would be to
find a way to loop through input types (checkbox, select, radio, text,
textarea, etc.), rather than handling each one explicitly. (And perhaps
you've done this; I've not looked at the code in question.)

Chip

On Wed, Jun 8, 2011 at 4:07 PM, Darren Slatten <darrenslatten at gmail.com>wrote:

>
> Anyway, that sort of question is kinda petty when you get right down
>> to it. Theme review is an ongoing process, and the guidelines will
>> change as needed. For now, I agree with the no-PHP-notices rule, as it
>> shows an attention to detail. I myself don't always check my plugins
>> to not produce notices, but it's something I know that I should do.
>>
>
> I completely agree. However, the theme I'm working on right now throws 100+
> notices in some situations. They are all due to undefined variables/indices
> that are used to store the theme's options. Unfortunately, the options panel
> only saves the option values that have been explicitly set (so for example,
> an option with an empty checkbox is not saved as false--it's simply left
> undefined). Then...throughout the entire theme, the author used a code
> pattern like this:
>
> if ( $themeslug_options['footer']['show_copyright_info'] )
>     themeslug_copyright();
>
> Basically, the author is working with the assumption that *undefined* will
> behave just like *false* (and honestly, I can't think of a relevant
> scenario where this assumption would cause problems).
>
> So in a case like this (keeping in mind that this theme has a huge amount
> of options), flushing out all the Undefined notices would require
> substantial changes to the code. What would be the best way to resolve this?
> The solutions I see are:
>
>
>
> Change all occurrences of this:
>
> if ( $foo['bar'] )
>
> ...to this:
>
> if ( isset( $foo['bar'] ) && $foo['bar'] )
>
>
>
> OR
>
>
>
> Rewrite the options panel (~3,000 lines of code).
>
>
>
> I'm guessing the latter solution is preferable?
>
> -Darren
>
>
>
>
>
> On Wed, Jun 8, 2011 at 1:56 PM, Otto <otto at ottodestruct.com> wrote:
>
>> On Wed, Jun 8, 2011 at 1:49 PM, Darren Slatten <darrenslatten at gmail.com>
>> wrote:
>> > I could be wrong, but I got the feeling that part of the question was
>> along
>> > the lines of:
>> >
>> > If PHP notices are not permitted, then why do some already-accepted
>> themes
>> > generate PHP notices?
>>
>> Obviously, no theme review process involving human judgment is going
>> to be perfect.
>>
>> In the end, you're just going to have to say "just because theme X has
>> the problem doesn't make it not actually a problem".
>>
>> Anyway, that sort of question is kinda petty when you get right down
>> to it. Theme review is an ongoing process, and the guidelines will
>> change as needed. For now, I agree with the no-PHP-notices rule, as it
>> shows an attention to detail. I myself don't always check my plugins
>> to not produce notices, but it's something I know that I should do.
>>
>> -Otto
>> _______________________________________________
>> theme-reviewers mailing list
>> theme-reviewers at lists.wordpress.org
>> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
>>
>
>
> _______________________________________________
> theme-reviewers mailing list
> theme-reviewers at lists.wordpress.org
> http://lists.wordpress.org/mailman/listinfo/theme-reviewers
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wordpress.org/pipermail/theme-reviewers/attachments/20110608/8c510b13/attachment.htm>