When you start installing drupal the very first screen that greets you is full of notices (see screenshot). I understand that this is in a lot of places in drupal, but the very first screen that the user sees should not have any. Also there is this page: "Write E_ALL compliant code (http://drupal.org/node/34341)".
The attached patch makes sure that all the variables are initialized or before accessing them. The way this is done for $element is probably not good. All the issets should be in one place, when the $element variable is built me think.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | notices_1.patch | 2.45 KB | RobRoy |
| #21 | install-notice_1.patch | 2.91 KB | webchick |
| #18 | install-notice_0.patch | 2.66 KB | webchick |
| #16 | install-notice.patch | 2.23 KB | webchick |
| #1 | notices.png | 78.87 KB | jax |
Comments
Comment #1
jax commentedThe screenshot.
Comment #2
jax commentedI'm going to bump this once because it seems important to me.
Anyone that has his webserver set up like mine sees all the notices. Moreover you cannot finish the installation with the notices because there are some redirects. I think this doesn't give a good impression to new users at all.
Maybe there should be some text in the INSTALL.txt or first update page about this instead of fixing them all so that new users at least know that they should turn the notices off.
Comment #3
webchickYes, this is a bloody annoying problem.
It involves form.inc changes to fix properly though, for which chx is the best person to review.
Comment #4
jax commentedOne solution would be to put a "php_value error_reporting 2039" (= E_ALL ~ E_NOTICE) in the .htaccess. Then all notices will be hidden.
Comment #5
RobRoy commentedExcept for solutions that don't allow .htaccess which may be the installations that are more likely to not have a suitable error_reporting E_ALL & ~E_NOTICE value to begin with. I think creating E_NOTICE compliant code is a good thing and doesn't really bloat it that much.
Comment #6
webchick-1 to cheating around this problem with .htaccess.
Huge +1 to actually fixing it.
I will try and review this tonight/tomorrow. There's nothing worse than trying to demo the kick-ass installer for someone, only to be assaulted with 5000 notices.
Comment #7
jax commentedThe thing is that you cannot go around putting isset()'s just anywhere. lyricnz is submitting a load of patches to remove notices, but that's just all it does (just like mine). If you try to access those same elements one line before the isset the notice is there again.
That's why I made the remark about $elements in my patch. The isset() is IMO totally misplaced in my patch. All properties of $elements and $forms should be set when that is built, not when it is first accessed and that is no simple feat to do and keep doing.
Comment #8
Gary Feldman commentedThis is just to get the terms undefined index and undefined variable into this issue, so that it's easier to find this issue by searching for those terms. I wasn't able to find this note by searching; I only found it by submitting a duplicate, which was closed with a pointer here.
Aside: One of the problems with screen shots is that they're not searchable. That doesn't mean not to use them, but keep in mind ways that others might try to search for your issue.
Comment #9
jax commentedAnother term that is often used for this kind of tickets is E_NOTICE. Maybe all of these http://www.google.com/search?q=site%3Adrupal.org+E_NOTICE+lyricnz could be grouped somewhere.
I also want to reiterate that putting isset()'s everywhere is not a good solution.
Comment #10
Gary Feldman commentedIt's not a complete solution, but is it bad? The function in which these errors occur have to do something, whether or not the calling function needs to be fixed.
So, for example, theme_markup has to check that the values are there (or else suppress the warnings explicitly), because it has no control over who calls it with what.
Comment #11
jax commentedYes, in my opinion, it is bad, very bad. The problem is that it is even possible to access unset array members/indexes/variables. In other languages (java/python) you get an exception if you try to do such thing.
You are navigating your site and you notice that when you do action foo it triggers a notice at line 345 on file index.php. You fix it by putting an isset on line 345. Now, there is no guarantee that any other action will also trigger a notice on that exact same variable on a line < 345. So someone else notices that action bar also triggers a notice in index.php on line 123 and fixes it. But the thing is it was on the same variable, so your fix has become irrelevant. And after a while your code is full of isset()'s everywhere hampering readability and performance.
Solving the notices on array keys/indexes kan be solved in 3 ways. One is to decide a place where to construct the arrays and make sure all members are set. For example when you create a module and you create form elements in your module, the core that calls the hook_form hook should clean up after the call and complete all the '#attribute' keys. So that when they're access later they no longer trigger notices.
Another option is to outlaw accessing array indexes immediatly. Instead you introduce a function get($array, $key, $default = '') which returns het value of $array[$key] if it's set and $default otherwise. (In python you have this function built-in on arrays and hashes).
The third options would be to put in big bold letters in the install.txt "TURN OFF NOTICES!!!".
I don't know how feasable all those options are but the easiest would be #3.
Comment #12
jax commentedYes, in my opinion, it is bad, very bad. The problem is that it is even possible to access unset array members/indexes/variables. In other languages (java/python) you get an exception if you try to do such thing.
You are navigating your site and you notice that when you do action foo it triggers a notice at line 345 on file index.php. You fix it by putting an isset on line 345. Now, there is no guarantee that any other action will also trigger a notice on that exact same variable on a line < 345. So someone else notices that action bar also triggers a notice in index.php on line 123 and fixes it. But the thing is it was on the same variable, so your fix has become irrelevant. And after a while your code is full of isset()'s everywhere hampering readability and performance.
Solving the notices on array keys/indexes kan be solved in 3 ways. One is to decide a place where to construct the arrays and make sure all members are set. For example when you create a module and you create form elements in your module, the core that calls the hook_form hook should clean up after the call and complete all the '#attribute' keys. So that when they're access later they no longer trigger notices.
Another option is to outlaw accessing array indexes immediatly. Instead you introduce a function get($array, $key, $default = '') which returns het value of $array[$key] if it's set and $default otherwise. (In python you have this function built-in on arrays and hashes).
The third options would be to put in big bold letters in the install.txt "TURN OFF NOTICES!!!".
I don't know how feasable all those options are but the easiest would be #3.
(hmmm.. triggered another bug apparently with < 345, check out the source of the page, submitting issue...)
Comment #13
webchickK, I've marked about two or three issues now as dupes of this. Going to try taking a look tonight.
Comment #14
webchickMore descriptive title. Also, the unicode notices are handled by http://drupal.org/node/85689#comment-154391
Comment #15
webchickComment #16
webchickHere's a patch from Eaton which attempts to fix these closer to their source. Still needs work because of:
Comment #17
webchickAlso, if password is left blank and the form is submitted:
Oddly, leaving user/hostname blank doesn't do this. Not sure why.
Comment #18
webchickHere's the remainder of Eaton's form.inc changes, which should finish the job.
The blank pass thing is not covered because a) it looks like it's going to be a royal pain to fix, and b) that's only a minor issue that can be picked up after the fact. This and http://drupal.org/node/85689, on the other hand, are required to remove the screen full of errors that appear when you first hit the install.php page.
Comment #19
webchickBah. Not done yet. There's some more stuff in other places:
install.php, with more than one profile:
After hitting submit on the database settings page:
(This actually generates a blank white page, apart from these errors and the usual "Headers already sent")
My brain hurts. Going to bed. :P If someone else would like to take these on, that would be awesome.
Comment #20
webchickThere are also a butt-load of other errors if you manually enabled E_ALL:
This per each module enabled:
And finally:
(the last one is simple.. changing $output .= to $output = ... not sure about the others.)
Ok. Really done with this now. ;)
Comment #21
webchickRe-roll of Eaton's FAPI fixes based on chx's input.
Comment #22
jax commentedWebchick, and everyone else, it is amazing the amount of work you have put into this. It is an achievement on its own.
But...
As long as there is no formal commitment to keep 'core' E_ALL compliant all your work is unnecessary and might hamper 'the project' in the long run. Even if core is E_ALL compliant you can never know if other code is moved somewhere which breaks your stuff.
I have been thinking about this for quite a while now, and I cannot find any solution that makes it possible to accept a isset() anywhere, unless it is with $_GET, $_POST or their family.
Abandon it, make drupal E_ALL ~ E_NOTICE compliant, and forget about the rest.
Comment #23
webchickI disagree.
First of all, because I think core should be E_ALL compliant, but that's entirely beside the point. ;)
The real reason is that THIS particular instance of E_ALL compliance is important, because common.inc (which has code in it which suppresses E_NOTICEs) hasn't been loaded yet since we don't yet have a database connection. Therefore, the only way for a user to suppress the errors during the installation phase is to edit php.ini, which the vast majority of users won't have the ability to do (and that is a total non-"fix" anyway).
For the other issues you are right, which is why I marked them minor. However, this one needs to get fixed before 5.0 ships, imo.
Comment #24
webchickBtw, my long-term solution to this problem is to lobby to get the patch here: http://drupal.org/node/99625 committed at the beginning of each release cycle. That way notices are caught at the moment patches are rolled and not way after the fact when it requires 100 issues to clean them up.
Comment #25
RobRoy commentedI agree that we should move towards E_ALL compliance. I'm behind this 99% (Margin of laziness: %1). Here is a re-roll of webchick's patch in #21 which took out one set of parens, changed an "else if" to "elseif", and fixes some dot code style. No real work was done here. :)
Comment #26
webchickPatch in #21 isn't ready yet... there's all the other notices that need to get tackled in install.inc and install.php, etc.
Comment #27
webchickMarking dupe of http://drupal.org/node/103250