Always report E_STRICT errors

-    $default = array_shift(array_keys($languages));
+    list($default) = array_keys($languages);
   }
   else {
     $languages = array(
       t('Already added languages') => $names,
       t('Languages not yet added') => _locale_prepare_predefined_list()
     );
-    $default = array_shift(array_keys($names));
+    list($default) = array_keys($names);
   }

Those looks like a wicked way of doing $default = key($languages);

+      $node->taxonomy[arg(3)] = new stdClass();
       $node->taxonomy[arg(3)]->vid = $vid;
       $node->taxonomy[arg(3)]->tid = arg(3);

Would be better as:

$node->taxonomy[arg(3)] = (object) array(
  'vid' => $vid,
  'tid' => arg(3),
);

And

+          $current = new stdClass();
           $current->tid = $tids[0];

better as:

  $current = (object) array(
    'tid' => $tids[0],
  );

Also see #325827: Avoid PHP strict timezone warnings by calling date_default_timezone_set in bootstrap to fix strict errors with date functions. I've run into those several times in tests.

By default, the tests run with 0 errors and 0 exceptions. So, I modified _drupal_error_handler in common.inc to report E_ALL and E_STRICT errors. I think we should set the error handler to E_STRICT during the development cycle. That should probably be part of this patch. Unfortunately, setting it to E_STRICT results in additional errors not covered by this patch.

This patch was accidentally committed along with #323528: UPDATE queries should not use aliases. http://drupal.org/cvs?commit=159769

It doesn't really matter. Let's mark this one as fixed for now. Thanks for the patch, Christian.

I get a "Creating default object from empty value" From the aggregator module when running tests. This patch fixes that warning.

Could you also throw a 'stdClass' type hint on that parameter while you're there as well?
function aggregator_form_feed(&$form_state, stdClass $feed = NULL) {

Done.

In that case, it would looks better with a !isset() instead of the empty().

Committed to CVS HEAD. Thanks.

How can we configure the test framework so that strict warnings always show up? :)

I seems our efforts are best spent getting these patches in then, instead of repeatedly chasing HEAD to be E_STRICT compliant. At least, that is my take on this situation.

Rerolled including fixes for lots more strict notices.

chasing HEAD.

Hmm that was weird my comment didn't show up.

chasing HEAD

Found a few more strict errors.

fix another strict notice in image module

chasing HEAD

Missed one strict notice

Re-roll and fix some non-static functions that should be static.

Re-roll and fix some non-static functions that should be static.

Found a few more strict notices.

fixed yet another drupal_render() issue, in book.module

reroll

Add a tweak from #561624: Strict error due to function signature incompatibility of TaxonomyTermController::cacheGet() -- pass $conditions thru to parent::cacheGet()

Reroll

Thanks for these clean-ups. However, I don't see anything in here that will keep the test bot running in E_STRICT mode and prevent you from having to re-roll this patch another 100,000 times throughout the development cycle.

In other words, can we fix #325827: Avoid PHP strict timezone warnings by calling date_default_timezone_set in bootstrap and #350543: Use E_STRICT error reporting as part of this patch?

Here I enabled E_STRICT reporting in .htaccess so it can catch compile-time strict warnings; calling error_reporting() at runtime doesn't have the same effect. Will mark #350543: Use E_STRICT error reporting as a duplicate. The only issue with .htaccess is we cannot use PHP constants E_ALL | E_STRICT. This page suggests using a large number since new error levels will be added over time: http://www.php.net/manual/en/errorfunc.configuration.php#ini.error-repor...

I also tweaked _drupal_log_error() to not display strict warnings if the error level is set to ERROR_REPORTING_DISPLAY_SOME (whereas before it only avoided displaying notices).

There may be some other strict warnings that test bot finds so let's see if this works...

Well that was spectacular. Looks like the test server doesn't have a time zone configured so I also have to merge in #325827: Avoid PHP strict timezone warnings by calling date_default_timezone_set in bootstrap

Note that unlike the rest of this patch, #325827: Avoid PHP strict timezone warnings by calling date_default_timezone_set in bootstrap results in a major functional change: any 3rd-party code that calls date/time functions will use a different time zone depending on the current user.

Marked #510436: drupal_render($function()) causes strict warnings as a duplicate.

hmm I could not reproduce this test failure

Rerolled

rerolled

Just FYI, I'm getting a huge number of these strict warnings while running the installation script with the drupal 7 dev tar file from 08-dec-2009. (I realize this is not a released version; plus I'm running a shaky version of Ubuntu, which might be the real problem.)

"Warning: date_default_timezone_get(): It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/Chicago' for 'CST/-6.0/no DST' instead in _install_configure_form() (line 1642 of /var/www/install.php)."

subscribing

Rerolled, chasing HEAD.

Patch works as advertised, warnings gone.

I tested the patch in #56 and it solves the problem from #668496: Timezone error on D7 install.

needed another reroll.

Bumping to critical. Without this patch, some users will see huge walls of red errors on install -- a major WTF.

Throwing an exception seems like a good idea to me. And this method should have some phpdoc.

I didn't want to blindly change array_shift(array_keys()) to key() because these aren't necessarily the same. array_shift(array_keys()) returns the first key, whereas key() returns the key of the current array position.

I'm curious if there's a rationale why

$current = (object) array(
  'tid' => $tids[0],
);

is better than

$current = new stdClass();
$current->tid = $tids[0];

? I find the latter easier to read.

+    $langcodes = array_keys($node->taxonomy_forums);
+    $langcode = array_shift($langcodes);

key() should be used, but reset() would be prudent first:

+    reset($node->taxonomy_forums);
+    $langcode = key($node->taxonomy_forums);

The fix for #325827: Avoid PHP strict timezone warnings by calling date_default_timezone_set in bootstrap and #668496: Timezone error on D7 install was merged into this patch so, would be a good idea to get it in soon, since anyone running PHP 5.3 is seeing this bug.

#62: strict.patch queued for re-testing.

Re: #65 and #67, I checked and there should be no side-effect of resetting the arrays, so I replaced the three instances of array_shift(array_keys()) with reset() key().

I added some phpdoc to FileTransfer::factory() and made it throw an exception.

Excellent. This removes all the date() issues when installing, as well.

fixed a couple more issues in drupal_web_test_case.php and color.test and queue for retesting

Hmm, if #671520: Curl and fragment URLs break testing goes in, it'll cause a conflict with the strtok change you made to the web test case. Could you revert that?

I can easily fix conflicts later, for now the patch won't test cleanly without this line. Meanwhile I already found some other failures that I still need to fix (running tests locally).

Sure, you're right of course, priorities. :)

ok, let's see if tests pass

That failure looked kinda like a fluke...

#76: strict.patch queued for re-testing.

Comment paging settings tests have been broken on HEAD for a while, for me at least.

Rerolled to accommodate errors.inc

Needed another reroll for install.core.inc

Will this work on Alpha2? 1st glance, revision #'s don't match Alpha2. Alpha2 definitely has problem.

#82: strict.patch queued for re-testing.

rerolled

#87: strict.patch queued for re-testing.

I won't second-guess webchick on the priority here, but let's remember that anyone using PHP gets deluged with errors without this patch, so it's pretty important if not critical...

This really should get in if we expect to be running on PHP 5.3.

Mac OS X Client/Server 10.6.x (Snow Leopard) come with PHP 5.3 preinstalled. So anyone installing this on a Mac will get this error which doesn't look very welcoming during install, and every time you open the user id=1 page, or any of the regional settings in the admin interface.

Just looking through the patch. Everything looks good, just a question about:

+++ .htaccess	7 Mar 2010 19:17:35 -0000
@@ -38,6 +38,8 @@ DirectoryIndex index.php index.html inde
   php_value mbstring.http_output            pass
   php_flag mbstring.encoding_translation    off
+  # Report all PHP errors, including compile-time errors.
+  php_value error_reporting                 -1
 </IfModule>

Hm, do we really want to do this? There was imho a reason that we respected the already set error_reporting configuration.

142 critical left. Go review some!

This patch presumes that, at least in the dev version of core, we now want to catch all errors, including compile-time errors. This was more-or-less suggested by Dries and webchick above.

We can always apply a patch to releases with different error reporting configuration (or logic, if it's in the code), as is done in d6. (Where is the release patch maintained anyways?)

Fixed a PHP warning in drupal_serve_page_from_cache(), line 1103 of bootstrap.inc, when using PHP 5.3:

strtotime(): It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/Los_Angeles' for 'PDT/-7.0/DST' instead

-  // Enforce E_ALL, but allow users to set levels not part of E_ALL.
-  error_reporting(E_ALL | error_reporting());
-

I'm not happy about removing this hunk; this was one of the big wins in terms of better control over error reporting.

Can we change it so that it allows /stricter/ error reporting, as well as less strict?

There's no reason to get rid of that hunk, other than that it doesn't do anything when error_reporting is already set to -1 by .htaccess.

How about we preserve this hunk, and remove or comment out the php_value error_reporting -1 in .htaccess when rolling a release? So strict error reporting would only be enabled in dev branch of core.

Fixed a test crash on PHP 5.3 using this logic: $function = version_compare(PHP_VERSION, '5.3', '>=') ? 'parent::setUp' : array($this, 'parent::setUp');

see http://www.php.net/manual/en/function.call-user-func-array.php#93744

All tests pass now on PHP 5.3 (Ubuntu Lucid)

I removed "~" from the file_create_url() tests, because it is encoded by rawurlencode() in PHP 5.2 but not in PHP 5.3, as per RFC 3986. See http://www.php.net/manual/en/function.rawurlencode.php#86506

Subscribing.

sub.

The diff looks good. What were the remaining issues?

I don't know of any remaining issues. Would be a good idea to get this reviewed and committed (in my biased opinion ;). FreeBSD Ports got PHP 5.3 today, Lucid Lynx will soon be released with PHP 5.3, Snow Leopard already has it. This patch started out as PHP 5.2 E_STRICT compatibility, i.e. compatibility with future versions of PHP, but now the future version is here! Last I checked, this patch was required for the test suite to pass on PHP 5.3

I catch some stricts and a hunk already applied on head, so I'll be providing a re-roll ASAP

ok, there were to failed files on the patch on actual HEAD, so I rerolled it(strict-v39.patch) to:

- avoid one already applied hunk on http://drupal.org/cvs?commit=347230
- no more need to change FieldTestCase::setUp() on call_user_func_array because it is no more used since it now follow DrupalWebTestCase::setUp way of passing parameters. See http://drupal.org/cvs?commit=346994

About the "other stricts" I mentioned, those are noticed after clearing the cache, and it seems to not be inside the test running, it is on listing the tests, so it seems unrelated, so I make another patch for that. I mean I'm really not sure if we want the second(avoid-strict-on-listing-after-clearing-cache.patch).

ok, loving bot, there was a change on ColorTestCase::testColor()(the diff), so I just re-do what it is done in the original patch.

In the other side, I see simpletest tests having the strict warnings about the setUp() definition, so I'm making this a unique patch.

Fixed two exceptions from #107.

here is the same hunk modified in #109, but this also includes the .htaccess change(missing in #109).

What I really want to ask is:

I changed all DrupalWebTestCase's setUp() definitions on its children to avoid the the strict about declaring a compatible method to setUp($modules = array()), but I did not touch the logic inside each child to pass $modules variable merged with the modules actually passed inside the child implementation, and it seem like it's really not necessary now, but to follow a better logic for this we should make a array_merge before calling parent method.

Any suggetions?

My suggestion is: I don't think setUp($modules = array()) should be used as the method signature, given that the variable number of string arguments form is much more common, and also because $modules aren't (yet) being passed to parent::setUp() in most cases.

agree with the solution, _a lot_ less intrusive than mine :-p, RTBCing this

I ran the tests on my local machine and found a couple more issues:

Non-static method called statically:

> @@ -394,7 +394,11 @@ class RdfCommentAttributesTestCase exten
>  
>    public function testAttributesInTeaser() {
>      $node = $this->drupalCreateNode(array('type' => 'article', 'uid' => 1, 'promote' => 1));
> -    CommentHelperCase::postComment($node, $this->randomName(), $this->randomName());
> +    $comment = array(
> +      'subject' => $this->randomName(),
> +      'comment_body[' . LANGUAGE_NONE . '][0][value]' => $this->randomName(),
> +    );
> +    $this->drupalPost('comment/reply/' . $node->nid, $comment, t('Save'));
>      $this->drupalGet('');
>      $comment_count_link = $this->xpath('//div[@about=:url]//a[contains(@property, "sioc:num_replies") and @rel=""]', array(':url' => url("node/$node->nid")));
>      $this->assertTrue(!empty($comment_count_link), t('Empty rel attribute found in comment count link.'));

and missing call to parent::setUp():

> @@ -1743,6 +1743,7 @@ class CompactModeTest extends DrupalWebT
>    }
>  
>    function setUp() {
> +    parent::setUp();
>      $admin_user = $this->drupalCreateUser(array('access administration pages'));
>      $this->drupalLogin($admin_user);
>    }

Committed to CVS HEAD. Thanks!

So one quick thing... does this require me to toggle on/off some setting prior to rolling official releases?

The hard-coded error reporting level in .htaccess should be commented out in a release. Otherwise contrib modules, PHP libraries, etc. that aren't E_STRICT compliant would trigger excessive error logging.

We just want to force testbot to report (and developers to notice) the E_STRICT warnings. Note: This is assuming that the testbot environments are actually using the .htaccess file! Otherwise, testbot could still be ignoring the E_STRICT warnings...

There is another E_STRICT warning in authorize.php.

"Strict warning: Only variables should be passed by reference in main() (line 161 of /Users/james/Sites/drupal/seven/authorize.php)."

Looks good. I hope if there are more we just file new issues rather than re-opening this forever?

By the way, I added some documentation here on default time zone: http://drupal.org/update/modules/6/7#date_default_timezone

Hm. I thought that this patch would inform us from now on if strict errors occurred? But I had to commit these two (plus the one in this issue) tonight:

#777138: Strict warning: Only variables should be passed by reference in install_settings_form() (line 849 of /includes/install.core.inc)
#777150: Strict warning: Only variables should be passed by reference in menu_set_active_trail() (line 2107 of /includes/menu.inc)

What's up with that? Can't we make testbot catch these..?

I don't think testbot tests authorize.php or install.php.

Heh, I had independently found and fixed the authorize.php warning at #777716: Strict warning: Only variables should be passed by reference in main() (line 161 of .../authorize.php) since I didn't know about this monster thread. And no, the test bot can't test authorize.php since we can't assume it's got an ftpd listening that we can connect to, etc.

Merlinofchaos pinged me in IRC tonight and pointed out that this change broke Views. We effectively now require E_STRICT compliance, post-post-post-post code freeze, without any advanced warning to contrib authors. He's not too happy. I'd much prefer to add a PHP 5.3 testing bot to the mix with this local modification which could report these problems, rather than passing the burden of code conformance to contrib developers.

I'm also not real plussed with the fact that despite this relatively invasive change, we're still getting E_STRICT errors... though I guess at least the authorize.php one was because we have no test coverage...

Anyway, I think it'd be a good idea to re-open this for discussion, and possible roll-back.

I assume you mean rolling back just this line: php_value error_reporting -1 in .htaccess? See #116, this should be commented out in a release.

Forcibly setting error_reporting to -1 in the development branch is the easiest way to make sure everyone is at the same level for purposes of testing and debugging, but there's no real harm in removing this line. Without it, some installations are going to be logging E_STRICT errors while others are not. PHP recommends setting error_reporting to E_ALL | E_STRICT in development and E_ALL & ~E_DEPRECATED in production.

While Drupal core itself should be E_STRICT compliant, contributed modules may not be E_STRICT compliant. So the recommended error_reporting setting for Drupal installations should presumably be E_ALL. This is already documented at http://drupal.org/node/34341 (which I updated for Drupal 7 under the assumption that php_value error_reporting -1 is going to be removed from any releases!)

Well, most contributed module authors are developing against HEAD, to take account for the latest changes. So removing this at release time doesn't do them any good. Once Drupal 7.0 is out, perhaps, but not during development.

So just remove it then :)

In a perfect world, all modules would be E_STRICT compliant, but as that's not going to happen, I'm OK with mfb's patch.

I think that certainly can happen. Drupal core used to not even be E_ALL compliant, ya know... Drupal 5 would make you *weep*! ;)

But we can't suddenly make E_STRICT a requirement for module developers 8 months after code freeze, when we're at alpha4/beta1. However, you have my full blessing to try and make this one of the first patches to get in for Drupal 8. :)

Committed to HEAD. Thanks!

Kay. :P

Patch for Drupal 8 is the reverse of #125.

@cha0s: haha! Nice one.

Oh right. ;)

Why can't I retest this patch... hmm

Not sure why it wasn't working. Here's a reroll, but it looks identical to me.

Edit: Oops, it's not that the patch wasn't applying... the code is just generating errors as this patch was intended to tease out...

Here's a quick patch with those exceptions resolved so that it shows up green.

Patch looks good, I generally switch my local environment between E_STRICT and E_ALL depending on what I'm working on, and have been bitten before forgetting to switch it back when working on core patches.

+++ b/.htaccess
@@ -38,6 +38,8 @@ DirectoryIndex index.php index.html index.htm
+  # Report all PHP errors, including compile-time errors.
+  php_value error_reporting                 -1

Why in .htaccess and not in drupal_environment_initialize() ?

21 days to next Drupal core point release.

Want to see if we get the same fails from #133 with this so not including tests.

With jbrown's fixes.

Bumping to major, because this is now an E_NOTICE on PHP 5.4:

PHP Warning:  Creating default object from empty value

This is going to be backported (only the fixes) to Drupal 7.

Tag

Additional fix for field UI test. It is definitely a shame about not being able to do nested function calls like that any more.

The reason to put it in .htaccess it to catch compile-time errors. By putting it in code, some E_STRICT errors will be missed (that other people see due to stricter error reporting in their PHP configuration).

.htaccess doesn't work for cli and other webservers though. Could we do both?

We could do both.

As far as the change to bootstrap, could we add the following lines instead of modifying the existing line?:

  // Report all PHP errors, including compile-time errors.
  error_reporting(-1);

That way it should be easier to identify these lines so they can be removed in beta and stable releases.

I'm sorry, but I don't see a solid discussion about compile-time errors in this entire issue. This issue was originally and always about increasing error_reporting from E_ALL to E_STRICT only.

I don't quite get what compile-time errors buy us. That's an entirely separate discussion to have.

So let's remove that line from .htaccess from this patch, and move the discussion about compile-time errors into a separate issue, please.

That makes sense to me, I opened #1211878: Report compile time errors. Putting this back to RTBC for now.

I don't think we can backport this to D7.

Having been a victim of a D6 Pressflow release that suddenly turned E_ALL notifications on (well, what they actually did was something similar to D7 which allowed the php.ini setting to "float up" and override the common.inc-mandated setting), I can say from experience that it's incredibly irritating to suddenly get logs filled with messages you didn't have before doing a simple point release update. Additionally, Drupal 7 was in development for 3 years, and has been out in use for 7 months, and E_ALL was the requirement the entire time that module developers were asked to adhere to. We can't just up and change that in 7.N; it'd be a new requirement, and thus an API change.

Sucks, because I don't want to mask PHP 5.4 errors, but I don't see a way of changing this in D7.

We are definitely backporting this. But only the fixes, as I mentioned in #140.

@webchick - for Drupal 8 (I think) and Drupal 7 (definitely), we are only talking about E_STRICT compliance for dev tarballs / git checkouts that aren't tags (i.e. we/you would need to switch it back to E_ALL for each point release). Just want to make sure we're talking about the same kind of backport before it's completely off the table.

Cross posted, adding back the tag since Damien's right we need to backport the compliance if not the reporting.

Opened #1211914: E_STRICT on test bots.

K, sorry, I wanted to hold off on this for a couple of days to get Dries's sign-off on changing 8.x's default error reporting to E_STRICT, since that's not strictly a D7 bug fix. Blessings have been granted, so:

Committed and pushed #143 to 8.x and the same patch without the first hunk to 7.x. Thanks!

We need a change notice about this.

Tried it out: http://drupal.org/node/1218314.

Class constructors must always have parentheses:

+    if (empty($poll)) {
+      $poll = new stdClass;
+    }

+      $variables['profile'][$field->name] = new stdClass;

http://drupal.org/coding-standards#constructors

Sure thing.

Committed and pushed to 8.x and 7.x. Thanks!