[theme-reviewers] THEME: Suffusion - 3.5.4

Fri Jul 9 01:32:19 UTC 2010

Simon,
After your statement last week about prioritization I was hoping to see some
activity regarding the approval of Suffusion. However I was quite
disappointed to see that let alone being approved, the Trac ticket wasn't
even assigned to anyone. It has now been almost 20 days since I submitted
version 3.5.4 and 5 days since I submitted 3.5.5.

In any case I cleaned up all the NOTICE statements that I could find and
resubmitted 3.5.6. I hope this at least gets assigned to someone - it is
quite irritating when users ask for bug-fixes that have been submitted weeks
ahead but not approved. This is the new Trac ticket:
http://themes.trac.wordpress.org/ticket/324

Sayontan.

On Sun, Jul 4, 2010 at 12:39 PM, Dave Jesch <davejesch at gmail.com> wrote:

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

-- 
Sayontan Sinha
http://mynethome.net | http://mynethome.net/blog
--
Lake Chargoggagoggmanchauggagoggchaubunagungamaugg - 45-letter Native
American name for Lake Webster, Webster, Massachusetts, meaning "You fish on
your side; I fish on my side; nobody fish in the middle."
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wordpress.org/pipermail/theme-reviewers/attachments/20100708/f4b3384a/attachment-0001.htm>