[theme-reviewers] THEME: Suffusion - 3.5.4

[Dave Jesch]

On Sat, Jul 3, 2010 at 6:52 PM, Sayontan Sinha <sayontan at gmail.com> wrote:

> Simon,
> This is exactly what I am trying to explain to you - the big picture.
>
> Take the options loop for example:
>
> foreach ($options as $value) {
>        if (!isset($suffusion_options[$
>>
>> value['id']])) {
>>        $$value['id'] = $value['std'];
>>    }
>>    else {
>>                $$value['id'] = $suffusion_options[$value['id']];
>>    }
>> }
>>
>>
> $options is a very big array that comes from theme-options.php, where *
> each* of the 400 odd elements with an id has an "std" value. The entire
> options framework is predicated upon this foundation (for the last 10
> months). You will never get the so-called error. Having your extra condition
> will cause it no difference - your example is more lines of code that will
> give you no advantage. You approach this from the angle that if
> $value['std'] is not set (which will never happen in that array),
> $$value['id'] will not get set, while the code in theme-options.php ensures
> that $value['std'] will be present for anything with an id. If it isn't, my
> theme will have bigger problems to handle than a debug message.
>
> You are trying to split hairs between isset and is_null. I am simply making
> use of the fact that if something is not set, its value is still null
> (though that will give you a notice, but only if you have debug turned on -
> never otherwise). It doesn't damage the code in any way and it surely
> doesn't affect how the theme runs. All these arrays that you are asking me
> to write conditions around, these are arrays that I control, not something
> that a user defines. Hence I know what is in them - checks such as this will
> only increase cycle times in so many cases without adding value. The next
> thing you know, you will ask me to check for isset around different global
> variables, because all my global variables are initiated through the above
> code and if I have an "isset" around $$value['id'], there is a high
> likelihood that a global variable will not be initialized according to you -
> a condition, which according to me will never occur in the theme.
>

I get what you're trying to do. But checking for the existence of an array
element does not take up that much additional time. Stop and consider what
happens when the array element doesn't exist. PHP raises an error condition.
It then writes something to the Apache log files (this happens whether or
not WordPress has WP_DEBUG turned on) and then attempts to recover. The
recovery mechanism is to evaluate the value as NULL. So yes, it has the
desired effect within your code, but the amount of overhead to get there
(raising error condition and especially writing to a log file) takes far
more cycles than checking to see if the array element exists beforehand. And
then once you've determined whether or not the array element exists, you're
copying it. This takes up more time and a lot more memory (as opposed to
using a reference). If you're really concerned about performance, try both
methods and benchmark them.

The other problem with the errors is that it's filling up the Apache log
files with a bunch of messages that don't serve any real purpose. Now if you
need to look in the logs, there are thousands of error messages that you
need to wade through that are just a distraction, making it very difficult
to find anything else that might be important. It's just good manners to
make your code as "quiet" as possible in the log files since you never know
what else is going on with the machine.

As far as the globals go, if there is a problem checking any unassigned
global values, that would also show up in the log file. If there are no such
messages in the logs, then the condition you're mentioning doesn't exists
and therefore doesn't need to be addressed: the globals are being
initialized before any reference to them, which is all that you need be
concerned about.

I don't know how else to explain this - it is such a simple concept that
> should raise no eyebrows, yet you seem to be bent upon trying to find flaws
> in it.
>

No one is trying to find flaws in your system. But errors are being written
to log files, which means that something potentially bad is happening. It
doesn't take much to change your code to remove the errors. No one is
telling you that your system needs to be changed, only that it should be
enhanced.  If all theme and plugin authors make their code as robust and
error free as possible, then WordPress itself will perform better -- which
is what we all want.


>
> Sayontan.
>
>
> On Sat, Jul 3, 2010 at 6:12 PM, Simon Prosser <pross at pross.org.uk> wrote:
>
>> On 04/07/2010 01:09, Sayontan Sinha wrote:
>> > Pick the very first one for example, which says that there is no index
>> > called "#horizontal-outer-widgets-1 a:hover/color" in
>> > theme-definitions.php. That is incorrect information and you really need
>> > to see the code to see how it works. This particular index has been
>> > defined multiple times, and there is an inheritance array defined, so if
>> > the variable is not defined in one place it picks it from the parent
>> etc.
>> first few examples:
>> if ($style_settings[$mapped_style] != null) {
>> $style_settings[$mapped_style] is not defined so you get the Notice.
>>
>> if ( isset($style_settings[$mapped_style]) ) {
>> does the same thing with no errors.
>>
>>
>> else {
>>        $parent = $style_settings["parent"];
>> $style_settings["parent"] is empty and causing the Notice, so again we
>> check:
>>
>>
>> else {
>>
>>        if ( isset( $style_settings["parent"] ) ) { $parent =
>> $style_settings["parent"]; }
>>
>> Most of the admin section errors come from this section:
>>
>> foreach ($options as $value) {
>>        if (!isset($suffusion_options[$value['id']])) {
>>        $$value['id'] = $value['std'];
>>    }
>>    else {
>>                $$value['id'] = $suffusion_options[$value['id']];
>>    }
>> }
>>
>> which can be written as:
>>
>> foreach ($options as $value) {
>> if ( isset( $value['id'] ) ) {
>>        if (!isset($suffusion_options[$value['id']])) {
>>        if ( isset( $value['std'] ) ) {
>>        $$value['id'] = $value['std'];
>>        }
>>    }
>>    else {
>>                $$value['id'] = $suffusion_options[$value['id']];
>>    }
>>        }
>> }
>>
>> The snippet above is used in numerous files to get the theme options and
>> is responsible for 99% of the notices displayed.
>>
>> --
>> MyBlog : http://www.pross.org.uk/
>> Plugins : http://www.pross.org.uk/plugins/
>> Themes: http://wordpress.org/extend/themes/profile/pross
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wordpress.org/pipermail/theme-reviewers/attachments/20100704/a6eb887d/attachment.htm>