TYPO3 Core

On which page? Is it reproducable? Please add some more details.

BTW, you can disable all PHP errors by adjusting displayErrors in TYPO3_CONF_VARS.

On each page and yes is reproducable.

By default, Typo3 sets error reporting to "E_ALL ^ E_NOTICE" in index.php, so naturally the notices are not shown.
If you change this to "E_ALL", you'll see more than enough notices.

Each of these notices points to a case of suboptimal coding, mostly accessing undefined array indexes.

I still cannot confirm this. Do you use PHP4 or PHP5?

Edit "index.php" and change the line

error_reporting(E_ALL ^ E_NOTICE);

to this:

error_reporting(E_ALL);

Also make sure that your php.ini has display_errors set to "on". Now use your web browser to access any page of your site.

See?

Hi,

please find a txt file attached to this bug which shows the first eight notices appearing when calling a page with ERROR_REPORTING set to E_ALL.

Sorry Jochen, I was wrong and you were right! I changed the setting in php.ini but didn't notice that it's been overwritten in index.php.

I am not sure if we should change all this. On the one hand, 700 notices are a lot - on the other hand the php interpreter already does error handling. if we checked for every error f.e with an "if" construct, our code would be a lot bigger and maybe therefore slower. We could try to prepend every variable where an undefined index could happen with @.
We could as well write a custom error handler which removes all "undefined index" notices and displays the others.
Just some thoughts on the topic... Greets Sebastian

many of the notices result from using an undefined index as a boolean to check if a TS variable is set, like this:

if ($conf['debugData']) { ...

this could be changed to

if (isset($conf['debugData'])) { ...

(which incidentily is more than three times faster....)

that is interesting. how did you benchmark this? can you provide us with some script or so testing that?

function microtime_float() {
list($usec, $sec) = explode(" ", microtime());
return ((float)$usec + (float)$sec);
}

$t1 = microtime_float();

for ($i = 1; $i < 1000000; $i++) {
if ($conf['debug']) {}
}

$t2 = microtime_float();

for ($i = 1; $i < 1000000; $i++) {
if (isset($conf['debug'])) {}
}

$t3 = microtime_float();

echo "old: ".($t2-$t1);
echo "\n<br>new: ".($t3-$t2);
echo "<br><br>difference: ".((($t2-$t1)/($t3-$t2))*100);

?>

The following test was done with error_reporting(E_ALL ^ E_NOTICE) as just E_ALL woudl spam the console and produce invalid results:

TESTING WITHOUT E_NOTICE (1 million array accesses each)

Testing access to set key without isset ...2.09166002274 seconds 2.09µs/access
Testing access to set key with isset ...2.9095480442 seconds 2.91µs/access
Testing access to NOT set key without isset ...3.39383101463 seconds 3.39µs/access
Testing access to NOT set key with isset ...1.73600912094 seconds 1.73µs/access

So accesing a SET key and doing isset() before costs 36%.
On the other hand you are right and accessing a NOT set key with isset() is 50% faster than without.

Now you have to do the following calculation:
About 3000 array accesses during one Page-View (estimated - PHP APD doesn't log array access, at least the current version)
700 of them access a not existing key.

so at the moment we have:
2300*2.09µs + 700*3.39µs = 7,19 ms

when we change ALL array accesses to use isset we have the following:
2300*2.91µs + 700*1.73µs = 7,90 ms

So a slight DECREASE in performance. But this calculation is only done on the estimation that there are about 3000 array accesses. I found currently no solution to count and overall them .... If you find a possibility to do this and your own calculation following the above scheme results in a speed increase of more than 10% then report this bug again.

For now I will close it.

This issue wasn't about a speed increase in the first place, but about proper programming. A PHP Notice is a sign of bad programming. Sebastian then remarked that the solution could make Typo3 slower, then Jochen showed in a benchmark that it won't have a serious negative impact on performance, thereby invalidating the only reason against fixing it.

One more reason why this is worth fixing (besides sloppy coding): When you use a debugger (such as the Zend debugger), the server will evaluate and send information about every Notice in the script, regardless if error_reporting is on or off. This makes it virtually impossible to use a debugger on a Typo3-based application.

Jochen showed in a benchmark that it won't have a serious negative impact on >performance

Well. But that argument has been deproved. It most probably WILL result in a negative impact.

And please keep in mind that this is a "NOTICE" and no "WARNING" .... "WARNINGS" are of course bad.

I know of other people who use the Zend debugger and don't have any problems using it properly despite the fact of NOTICEs generated. I guess you have to disable the handling of NOTICEs somehow by configuring you Zend Studio. You should ask how to do this on one of the mailinglists (dev would be ok).

I don't close it again because I will simply ignore this issue for now. Either change it yourself and tell us the results (if you get an overall speed increase we will of course consider it) but the argument that a NOTICE is a sign of bad programming is simply not true for me.

greets,
Bernhard

Hello Bernhard,

I read this thread for some time and I already posted some hints on this annoyance but regarding the meaning of a PHP NOTICE I have to concede with Daniel Kyak. Please have a look to PHP manual you can find at

http://de2.php.net/errorfunc

There is a paragraph explaining notices saying:

"...NOTICE messages will warn you about possible bugs in your code. For example, use of unassigned values is warned. It is extremely useful to find typos and to save time for debugging. NOTICE messages will warn you about bad style..."

From my point of view notices should be cleaned up since it is no good idea just to ignore a message that something is not correct in your code. We have to be aware of the fact that a notice means that the php compiler found some code which it can't interpret but he guesses something for you. The PHP developers can decide to change the guessing behaviour any time they want to so we should not trust in the fact that our code works for now.

I put it to my private task list to cleanup the sourcecode and to eliminate those notices and give you feedback about my results.

Greetz,
Michael Kraus

Hallo Bernhard,

I just stumbled upon this bug and would really appreciate, if that would be fixed.

It's absolutely bad programming to generate hundreds of NOTICES and as far as performance is involved, generating so many notices of course costs lot of additional memory for the PHP process to place those on the "stack". Moreover the PHP interpreter needs to guess what the programmer is really trying to do, so I would not rely on a dump interpreter to guess, what I was really trying to do in my script.

many of the notices result from using an undefined index as a boolean to check if a TS variable is set, like this:

if ($conf['debugData']) { ...

this could be changed to

if (isset($conf['debugData'])) { ...

Maybe it would be a good idea to use empty() instead of isset() if a set, but empty variable will have the same meaning as an unset one. I guess this is the case for many config options.

Another note:

The "undefined indicies" (or "undefined variables") can be a security risk if register_globals is still enabled (like it still is on many mass-hosting servers :-( )

The "most famous" example for this:

if ($password == "secret") $login = 1;
if ($login == 1) { # show private data
}

Now call this script using &login=1 and register_globals on...

Hi,
of course everybody is welcomed to send patches removing notices, these will be checked then. However, notice that
if($config['debugValue']) and if(isset($config['debugValue'])) is NOT the same. To have the same behavior, one would have to use if(isset($config['debugValue']) && $config['debugValue'])
Of course you can say this is "bad style" when there are notices, but I don't think the last line in my example is easier to read.
I'd favor easiness of code reading over PHP notices...
Greets, Sebastian

It's absolutely bad programming to generate hundreds of NOTICES and as far as performance is involved, generating so many notices
of course costs lot of additional memory for the PHP process to place those on the "stack". Moreover the PHP interpreter needs to
guess what the programmer is really trying to do, so I would not rely on a dump interpreter to guess, what I was really trying to do in
my script.

I wonder how anyone of you notices the notices? To me it seems, that you have to explicitely enable the notices output because TYPO3 already disables it.

There is really no problem with that check:
- no memory is needed because they are not even added to that "stack"
- changing it causes a slowdown
- it is not even easier to read

I really don't see why we are discussing about such minor things when there are so many other interesting things to do.

Sebastian:

To have the same behavior, one would have to use
if(isset($config['debugValue']) && $config['debugValue'])

or just if(!empty($config['debugValue'])

empty() only returns true if a variable
- is set and
- it is != "" ö and != 0

I'd favor easiness of code reading over PHP notices...

I favor easy debugging - and activating PHP notices makes debugging of typos in variable names etc. much more easy ;-)

Michael:

I really don't see why we are discussing about such minor things when
there are so many other interesting things to do.

These "minor things" can even be security holes (variable injection using URL parameters) if register_globals is still on.

I really don't see why we are discussing about such minor things when there are so many other interesting things to do.

We are discussing about such minor things, because doing the boring work of cleaning up such issues is what turns an interesting, fancy and glossy design study into production quality software. If we want TYPO3 to be high quality software, such issues must be fixed. Just my 0,02€

TYPO3 is high quality software. Feel free to compare with other big(!) solutions and how many workarounds the require.

If you can't live with that, then just go ahead and send me a patch!

Sorry Michael, but generating hundreds of notices per page isn't what I would call high quality software.

In many "notice-cases" PHP has to reinterpret variables not existing and change them to something the parser thinks the programmer wanted to do - to me this is bullshit as I would not want to rely on the guessing-factor of a stupid interpreter.

Please don't argue using the performance factor as this is no real argument. Planes would probably fly faster if you would disable all security functions, but would you like plane manufacturers to really remove them because of "performance" issues?

Dear all, please calm down and stop this discussion about "whether TYPO3 is high qualitity software" or move it to private mail. This is not helping anyone.

If I read the comments so far correctly, there are differing opinions whether the core developers should spend time writing a patch for this.

Yet, as Michael said, this should not stop anyone who cares about these warnings from writing and submitting a patch.

Whoever does not like this situation: Feel free to write a patch and post it here. The problem is cosmetical and therefore we have set our priorities different. If you present a patch which is ready to be applied, I'm willing to commit it.

Sorry Oliver, I didn't notice that you already replied on that... :-)

Dear core developers,

would you be so kind and tell me how such a patch should look like? Unfortunatly this information is missing and I want to do the patch the right way.

In reply to Oliver: Calming down is basically the right way due to the fact that this doesn't solve the problem. But if you communicated this way after my post no. 0003383 we could already have the patch available.

I hope you understand me the way I mean it - I'm not interested in any confrontation but in a good result.

Regards,
Michael

Hi Michael (Kraus),
writing a patch is quite easy. You copy the core directory and name it for example TYPO3core_orig - so you have an "original core" and a working copy of it. Change everything which needs to be changed in the working copy, and then do a

diff -ru TYPO3core_orig TYPO3core_working > name_of_the_patchfile.patch

It would be of great help to use the latest beta or the CVS version as a basis.
Greets, Sebastian

Example#1 A simple empty() / isset() comparison.

$var = 0;

// Evaluates to true because $var is empty
if (empty($var)) {
echo '$var is either 0, empty, or not set at all';
}

// Evaluates as true because $var is set
if (isset($var)) {
echo '$var is set even though it is empty';
}
?>

I am now closing this issue since no one will come up with one patch for all the thousands of notices in the core.
Fixing single problems per issue - like in #22526 - can be the way to go.

Resolved, suspended.

I think we should close this issue. There are still tons of NOTICE level php messages throughout the core, but we can't handle them with a 'one for all' ticket.

Instead I'd please everybody who wants to fix some of them to open single issues, eg. to fix a specific systematic problem throughout the core like the missing is_array() check before foreach hook iteration.