PHP strict warnings when running tests
c960657 - December 17, 2008 - 23:54
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | other |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
Almost all tests pass with E_STRICT warnings enabled. This patch fixes the test. There may be more E_STRICT violations in Drupal, but if there is, these aren't covered by tests.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| strict-1.patch | 4.37 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
<?php- $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);<?php+ $node->taxonomy[arg(3)] = new stdClass();
$node->taxonomy[arg(3)]->vid = $vid;
$node->taxonomy[arg(3)]->tid = arg(3);
?>
Would be better as:
<?php$node->taxonomy[arg(3)] = (object) array(
'vid' => $vid,
'tid' => arg(3),
);
?>
And
<?php+ $current = new stdClass();
$current->tid = $tids[0];
?>
better as:
<?php$current = (object) array(
'tid' => $tids[0],
);
?>
#2
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.
#3
This fixes the points raised in #1. Thanks for your prompt review.
#4
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.
#5
This patch was accidentally committed along with #323528: UPDATE queries should not use aliases. http://drupal.org/cvs?commit=159769
#6
It doesn't really matter. Let's mark this one as fixed for now. Thanks for the patch, Christian.
#7
Not that you cannot simply replace
if ($error_level & error_reporting())withif ($error_level & (E_ALL | E_STRICT))in _drupal_error_handler() – that would cause errors muted with @ to pop up which is probably not intended.With this patch and error_reporting set to E_ALL | E_STRICT in .htaccess, I don't see any strict errors when running the test suite.
WRT using E_STRICT during the development cycle I created #350543: Use E_STRICT error reporting.
#8
I get a "Creating default object from empty value" From the aggregator module when running tests. This patch fixes that warning.
#9
#10
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) {#11
Done.
#12
In that case, it would looks better with a !isset() instead of the empty().
#13
Changed to isset(), and fixed prepareQuery() in sqlite that caused a strict error during install.
#14
Committed to CVS HEAD. Thanks.
#15
Fix for more strict warnings introduced with #348201: Multiple load files, #341910: file_space_used() not checking properly checking bitmapped status value (adds unit tests) and #8: Let users cancel their accounts. With this patch, the whole suite again runs without strict warnings.
#17
Updated with some fixes for the
drupal_render(node_build($node))pattern.#18
How can we configure the test framework so that strict warnings always show up? :)
#19
When #325827: Avoid PHP strict timezone warnings by calling date_default_timezone_set in bootstrap is fixed, we can enable E_STRICT permanently in #350543: Use E_STRICT error reporting, and then the test framework will automatically detect strict errors.
In the meantime I guess it is possible to enable E_STRICT error reporting on the testbot machines using php.ini.
#20
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.
#21
The last submitted patch failed testing.
#22
Rerolled including fixes for lots more strict notices.
#23
The last submitted patch failed testing.
#24
chasing HEAD.
#25
Hmm that was weird my comment didn't show up.
#26
chasing HEAD
#27
Found a few more strict errors.
#28
fix another strict notice in image module
#29
chasing HEAD
#30
Missed one strict notice
#31
The last submitted patch failed testing.
#32
Re-roll and fix some non-static functions that should be static.
#33
Re-roll and fix some non-static functions that should be static.
#34
Found a few more strict notices.
#35
The last submitted patch failed testing.
#36
fixed yet another drupal_render() issue, in book.module
#37
The last submitted patch failed testing.
#38
reroll
#39
Add a tweak from #561624: Strict error due to function signature incompatibility of TaxonomyTermController::cacheGet() -- pass $conditions thru to parent::cacheGet()
#40
The last submitted patch failed testing.
#41
Reroll
#42
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?
#43
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...
#44
The last submitted patch failed testing.
#45
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.
#46
Marked #510436: drupal_render($function()) causes strict warnings as a duplicate.
#47
The last submitted patch failed testing.
#48
hmm I could not reproduce this test failure
#49
The last submitted patch failed testing.
#50
Rerolled
#51
The last submitted patch failed testing.
#52
rerolled