Posted by eMPee584 on August 12, 2009 at 10:37am
| Project: | Content Management Filter |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
The whole time i was thinking that my modification of index.php:
<?php
error_reporting(E_ALL);
ini_set('display_errors', TRUE);
ini_set('display_startup_errors', TRUE);
?><?php
function cmf_help($path, $arg) {
switch ($section) {
case "admin/help#cmf":
$output = '<p>'. t("This module adds an easier way for administrators to filter the content on a Drupal site for administration purposes.") .'</p>';
$output .= '<p>'. t("It's an improvement over the content page in the administration area of Drupal. It can show on the same page nodes and comments and adds new filters like role and author.") .'</p>';
break;
}
return $output;
}
?>break; is absolutly unnecessary and the double quotes in the compared string cost extra CPU cycles because double quotes are being evaluated for variable expansion, while single quoted strings are not.Does this really mean that all the time you have been developing this module, you never cared to enable error logging to screen neither looked at the watchdog table? And 1000+ users also have not?
Monitoring error output is absolutly crucial when programming. And making sure braindead code is not deployed to thousand(s) of live sites is definitly one of module maintainer's core duties! In any other programming language, hundreds of contrib modules WOULDN'T EVEN COMPILE! Seriously i am both shocked and disgusted by the simple fact that so many developers don't care about code quality, nevermind cosmetic coding standards.
Anyway, patch attached.
| Attachment | Size |
|---|---|
| cmf-fix-hook-help.patch | 1.08 KB |
Comments
#1
Oh and speaking of code cosmetics, the module is full of whitespace at end of line - which you would have noticed if you cared to run the Code Review module a single time.
#2
First, insulting the developers is not a good way to get a patch looked at. To take your tone: If you had bothered to read the issue queue, which every adopter should do before submitting a new issue, you would have found #517880: Coding standards which tells you that Coder has been run and those end-of-line spaces were not flagged.
Most adopters run on servers that are set up for production usage, so errors are not logged to the screen. My test site does show them on the screen and I am not seeing the errors you mention.
The performance difference between single and double quotes is so small that even core occasionally violates that "rule".
While the examples for hook_help did change in D6, what the actual parameter is called is immaterial to the correct usage of the hook.
Since the module is obviously operating for "over 1000 users," this is not "critical."
I don't know about the original maintainers, but I definitely care about code quality, which is why I will now ignore your insulting tone and actually look at your patch and make appropriate changes.
#3
Committed to 6.x-1.x-dev.
#4
Well please understand that i have been busy fixing this kind of bugs in too many modules to stay calm anymore. Nevertheless i take great care not to personally insult anyone, if i failed to do that please cite the part you find insulting.
Well i hope you do acknowledge this code is always checking an undefined variable, then returning another undefined variable. As said, with any other programming language this would not compile!
If indeed you have error logging to screen activated on your testing site, maybe the three lines put at the beginning of my index.php actually do make a difference. Please revert this fix, paste those lines into index.php and report back if you still do not see the errors. If it does make a difference, i'll create a small module implementing patchdoq module's hook_patch which adds these statements, it might be a server setting that makes the difference, that would explain the lack of awareness of and provide a bit of an excuse for these bugs in contrib code,
About the white space, i did only quickly skip over that when i did not care about the white space. In any case, the module's full of white space at EOL, which is what coder module does indeed complain about *right now*, maybe they were introduced by an editor not suitable for coding. Myself i am using KDE's Kate which is perfect for this task, you might check it out for yourself.
Oh and regarding the issue status, citing yched from #331350: PHP notices:
Admittedly it's not 10s but two on every page, however that is on EVERY page => grave digging..
Just having a look at #291026: change E_NOTICE to warning and allow selection of error level, might support my thesis that the above code in index.php really is required on many servers to see E_ALL output. Please would you be so kind and confirm that? Again, if that is a case, i owe an apology for the tone.
#5
I set my settings.php to E_ALL and display_errors to true. I found some undefined variables and committed a fix. I also found a way in my editor to find the line-ending-spaces and got rid of them. I fixed the hook_help. If there are any other undefined variables, it would help to have the error text pasted here.
#6
Ok so indeed fact is, most servers people are developing drupal code on are not set to E_ALL, causing module devs to stay unaware of bugs forever. Drupal itself does not mingle with this settings.
That explains it, but is bad. And this code was by far not the most senseless i've seen, often, APIs get proverbially raped by contrib code..
So please excuse my tone again, as mentioned this has cost me hundreds of hours of busy debugging that'd better be spent creating content for the site.
Now what to do, have you got an idea how to raise awareness about error_reporting(E_ALL); ini_set('display_errors', TRUE);?
#7
oh and btw, is there a reason you didn't simply commit my patch? the parameter to hook_help is indeed called $path now, a switch statement with a single alternative is senseless, and the double quoted string costs "many" CPU cycles to be wasted. If it is because of the social issue (you felt intimidated by the harsh tone of my initial report and wanted to use your own instead of my code), that is why the current drupal development model sucks most.
#8
In the major patch for #543864: Make it easier to add new filters (and freeze 5.x) I have added more help, so the switch is useful. That patch also changes to $path. I had already developed most of that patch when you opened this issue, so I had to save it off and work from a temporary copy of the module to get any fix committed; I simply went with the fewest changes at the time. I am waiting on introfini to review that big patch before committing it. I think you will find the module improved when it comes out. If you can't wait, send me a note with my contact form and I will send you that version in zipped form.
#9
Try as I might, I cannot get "undefined variable" to show up on the screen. I have "error_reporting" set to 32767, which should get everything, including E_STRICT.
#10
Automatically closed -- issue fixed for 2 weeks with no activity.