Make Skinr E_ALL compliant

Did you run update.php?

I'm seeing this on a fresh install too

I just looked at the code. This is an actual bug.

There's no check on the array index. This can probably be fixed with an isset in certain locations.

Anyone want to write a patch? :)

These are just notices not errors and this still need to be fixed properly but one way around it is to set php.in error_reporting = E_ALL & ~E_NOTICE .

Drupal 6 is E_ALL compliance but there's a lot of contrib module that aren't.

That being said I'm going to change the Issue title to something more appropriate.

I cannot reproduce these at all, and I have E_ALL set.

@jacine, yep. You're absolutely right. I did confirm this problem before but I think recent changes the the skin_skinset fix it. I think it was something related the the static cache not being used. I did a quick search but didn't find it.

But I guess we can mark this one closed.

Ok, good, because I thought I was going insane. I tried for a while to reproduce, and nothing.

Thanks :)

Sorry to reopen, but I'm also getting many notice lines -the same notices of this issue- in fresh installs of the current Skinr 6.x-2.x-dev (Sep 28, 2010). For example:

notice: Undefined property: stdClass::$status in /var/www/clients/client0/web7/web/sites/all/modules/skinr/skinr.module on line 865.
(...)
notice: Undefined index: garland in /var/www/clients/client0/web7/web/sites/all/modules/skinr/skinr.module on line 929.

These are caused by:

skinr.module, line 865, function skinr_skinsets

        $skinset->status = $theme->status ? 1 : 0;

skinr.module, line 929, function skinr_skinset_statuses

  return $statuses[$skinset_name];

Drupal documentation:
Write E_ALL compliant code (6.x, 7.x)

Related issues:
Let's make Drupal 6 E_ALL compliant (6.x)
E_ALL compliance during installation (6.x)
E_ALL compliance: remove notices on installation screens (5.x)

Those are core issues, but include ideas that can be applied to modules, for development and released versions.

Thanks @juan_g. I had tried setting this in settings.php and .htaccess and still wasn't getting the errors, so today I hacked core, and was finally able to see these.

Here's a patch.

Thank you very much, the patch has fixed it by using isset; no more notices left. Will this fix be included in the next Skinr 6.x-2.x, dev or alpha? :)

Unless other testers find something not correct, and since it's just applying isset, I think this is RTBC.

Tested with Skinr, Skinr UI, and dependencies enabled.

There's an extra $output variable that's not really being used

This one? (skinr_ui.admin.inc file):

function theme_skinr_ui_admin_skins($form) {
+  $output = '';

I think that looks ok. It's used some lines later in the same function:

  $output .= drupal_render($form['options']);

(...)

  // Output table.
  $output .= theme('table', $headers, $rows);

  $output .= drupal_render($form);

  return $output;

@ericduran That's for the admin page that lists the skinsets. I was getting a notice there when there where no skinsets on $output.

I have a feeling there are more of these lurking around, but I committed this to dev. Now I need to port this to 1.x-dev and D7.

@jacine, cool. :) I'm working on the other issues :-)

Depending on the case, something to consider is !empty() :

The difference between isset() and !empty() is that unlike !empty(), isset() will return TRUE even if the variable is set to an empty string or zero. In order to decide which one to use, consider whether '' or 0 are valid and expected values for your variable.

(Write E_ALL compliant code)

I read that, and also this:

The function isset() returns TRUE when the variable is set to 0, but FALSE if the variable is set to NULL. In some cases, is_null() is a better choice, especially when testing the value of a variable returned by an SQL query.

I think isset() was the right choice, but TBH, I'm not sure.

Reading PHP docs, I've just made this boolean table. Not completely sure, but it seems at least mostly right. Please correct me if you see any mistake.

It depends on what is exactly needed, but if(!empty($var)) looks like the replacement for if($var) with the most similar behavior, without "undefined" notices.

In general (and in this case) !empty() is better to use than isset(). They did a bunch of testing on it, and it's apparently faster than isset().

The reason to use !empty() in this instance is because the default parameter value, if nothing is passed, is FALSE. FALSE should not trigger the refresh (which it would if you use isset).

Thanks @moonray.

This needs work then.

+++ skinr.module	Working Copy
@@ -862,7 +862,7 @@
-        $skinset->status = $theme->status ? 1 : 0;
+        $skinset->status = isset($theme->status) ? 1 : 0;

A theme's status should always be set, so I do not understand this isset().

+++ skinr.module	Working Copy
@@ -915,7 +915,7 @@
 function skinr_skinset_statuses($skinset_name, $refresh = FALSE) {
...
-  if ($refresh) {
+  if (isset($refresh)) {

The function signature defines a default of FALSE for $refresh, so $refresh should never be not set --- unless something, anything, calls this function with a second argument of NULL, which most likely is not the case.

Powered by Dreditor.

Actually a theme's status isn't always set, apparently (though why that is, I have no idea). See comment #11.
I think that should be a check for !empty() instead of isset(), though.

The $refresh check should indeed not be set.

None of these appear in the D7 version, though. These are all D6 notices.

yeah, I can't get these notices to show in drupal 7

Alright, moving back to D6 since this isn't an issue in D7.

From reading it seems like that status here is that its pending a patch to 1.x-dev and the current code needs some revision? Like the use of empty() over isset() and The $refresh check should indeed not be set as Moonray said. With some direction I could try and help out unless its already being done or out of my capability.

Settings back to D6 since none of the issues reported above apply to code in D7.

Any code improvements mentioned above, like isset() vs !empty(), aren't crucial here. No notices appear anymore, so closing.