Plenty of E_ALL notices in modules developed on installation with Drupal tarball [#761780]

Comment #1

jennifer.chang CreditAttribution: jennifer.chang commented 4 April 2010 at 16:23

notice: Undefined property: stdClass::$tableofcontents_toc_automatic in tableofcontents.module on line 124.
notice: Undefined offset: 1 in tableofcontents.pages.inc on line 454.

when previewing and saving node.

Log in or register to post comments

Comment #2

jennifer.chang CreditAttribution: jennifer.chang commented 4 April 2010 at 16:24

notice: Undefined property: stdClass::$tableofcontents_toc_automatic in tableofcontents.pages.inc on line 640 when viewing a node without TOC!

Log in or register to post comments

Comment #3

jennifer.chang CreditAttribution: jennifer.chang commented 4 April 2010 at 17:12

notice: Undefined index: nid in tableofcontents.admin.inc on line 307.
notice: Undefined index: type in tableofcontents.admin.inc on line 316.
after visting the 'outline' tab (book.module) of a node.

Log in or register to post comments

Comment #4

AlexisWilke CreditAttribution: AlexisWilke commented 5 April 2010 at 09:53

Version:

6.x-3.5

» 6.x-3.x-dev

Hi Jennifer,

I could not replicate anyone of those... I wrote fixes for all of them but since I could not see them in the first place, I have no clue whether it was properly fixed. Please, try the -dev once available (within 12h) and let me know if that works for you.

Thank you.
Alexis Wilke

Log in or register to post comments

Comment #5

jennifer.chang CreditAttribution: jennifer.chang commented 5 April 2010 at 16:05

Thanks for the prompt feedback.

The latest -dev does not fix this one, at least:
notice: Undefined property: stdClass::$tableofcontents_toc_automatic in tableofcontents.module on line 121.

Setting to NULL amounts to unsetting the variable. Maybe you should set to an empty string ''.
Hmm... I just tried and even setting to '' (line 138) does nothing: there is still an error.

I'm now running the latest -dev (DRUPAL-6--3). I only need to do cvs udpate to get the latest code.

The fact that you do not see those notices shows that your development platform is not optimized for coding.
Whatever we are doing on the site, this notice #761770: Use of undefined constant TABLEOFCONTENTS_REMOVE_PATTERN - would show on every page. So you can use it to make sure your development platform is properly set up:
check in Drupal that error messages are set to display on screen as well as in the logs.
check your php installation (php.ini?) to make sure that E_ALL notices are reported. There is a setting that hides them. E_ALL should always be noted on a computer used for development. The most convenient is to use #761770 to test this.

Thanks again for the module and the quick feedback. I have the latest code and I'll use this issue to report other notices, if I find them in the next day or two.

Log in or register to post comments

Comment #6

jennifer.chang CreditAttribution: jennifer.chang commented 5 April 2010 at 16:23

A new one, after editing and saving a node (with latest code):
notice: Undefined offset: 1 in tableofcontents.pages.inc on line 454.

Previously, I had been playing with the filter settings:
admin/settings/filters/2/configure
what's changed was I configured it to add a TOC automatically and unchecked the option 'remove the TOC tags from the node summary.'

Log in or register to post comments

Comment #7

jennifer.chang CreditAttribution: jennifer.chang commented 6 April 2010 at 09:21

Those two still exist when saving a new node:

* notice: Undefined property: stdClass::$tableofcontents_toc_automatic in tableofcontents.module on line 121.
* notice: Undefined property: stdClass::$tableofcontents_toc_automatic in tableofcontents.module on line 124.

Log in or register to post comments

Comment #8

AlexisWilke CreditAttribution: AlexisWilke commented 10 April 2010 at 10:19

Hi Jennifer,

Okay, I have applied a few more fixes. Let me know how it works now. 8-)

Thank you.
Alexis

Log in or register to post comments

Comment #9

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 03:41

Thanks. The last ones I reported seem to have been fixed in 6--3-dev.

I just noticed another one right now while testing:

notice: Undefined property: TOC::$id in tableofcontents.pages.inc on line 832.
when editing and saving a node (the node type is set to have automatic TOC at the top.

Log in or register to post comments

Comment #10

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 03:44

Where you able to adjust your dev platform to report such notices? Otherwise this will be a recurring problem with this module.

Log in or register to post comments

Comment #11

AlexisWilke CreditAttribution: AlexisWilke commented 11 April 2010 at 04:22

Hi Jennifer,

I added a fix for the error on line 832.

I have turned on the E_NOTICE messages and had to fix many errors in many modules, but 0 errors from tableofcontents shown up. Dunno why... I use many modules on my development site... so maybe one of them prevents the errors.

I also edited the Core error module which would not report the E_NOTICE errors. And my PHP is always setup to show really bad errors in the browser (even on my production server.) And I still get no errors from the tableofcontents.

Now, maybe you could learn to write patches... 8-)

Thank you.
Alexis

Log in or register to post comments

Comment #12

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 04:33

Status:

Active

» Fixed

Thank you Alexis. Notice in #9 is fixed :)

It's strange you're not seeing those notices. I am not sure which core error module you're referring to.
Thanks for promptly fixing those errors. As far as I am aware, everything is fixed here.
If I find anything else, I'll reopen... maybe with a patch :)

Log in or register to post comments

Comment #13

AlexisWilke CreditAttribution: AlexisWilke commented 11 April 2010 at 04:39

Assigned:

Unassigned

» AlexisWilke

Jennifer,

Here:

// $Id: common.inc,v 1.756.2.79 2010/03/04 00:15:28 goba Exp $
[...]
function drupal_error_handler($errno, $message, $filename, $line, $context) {
  // If the @ error suppression operator was used, error_reporting will have
  // been temporarily set to 0.
  if (error_reporting() == 0) {
    return;
  }

  if ($errno & (E_ALL ^ E_DEPRECATED ^ E_NOTICE)) {
[...]

Thank you.
Alexis

Log in or register to post comments

Comment #14

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 04:44

I have:

  if ($errno & (E_ALL ^ E_DEPRECATED)) {

in my unpatched Drupal installation. Did you add ^ E_NOTICE ?

Log in or register to post comments

Comment #15

AlexisWilke CreditAttribution: AlexisWilke commented 11 April 2010 at 04:49

That's what I have in Drupal 6.16. I checked everything else and could not find a call that would clear the E_NOTICE flag other than that one line. I use the tarball though, not the CVS version of Drupal. They may have some magic in there? 8-)

Log in or register to post comments

Comment #16

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 04:59

I think we found the culprit. I checkout from CVS.

Apparently that line is updated back and forth before and after each release:
http://drupalcode.org/viewvc/drupal/drupal/includes/common.inc?view=log&...
It's obviously done on purpose so that developers see the notices, but not regular users.

The morale of the story: we should not develop using a Drupal platform installed from a tarball.
I was wondering why I was seeing so many notices in so many modules.... and I was apparently the only one to see them :(

I think I'll patch my production platform accordingly!

Log in or register to post comments

Comment #17

AlexisWilke CreditAttribution: AlexisWilke commented 11 April 2010 at 05:15

I wonder whether there are other differences in the tarball... 8-}

Log in or register to post comments

Comment #18

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 05:39

Title:	Plenty of E_ALL notices in modules developed on installation with Drupal tarball	» Plenty of E_ALL notices
Project:	Documentation	» Table of Contents
Version:		» 6.x-3.x-dev
Component:	Other documentation issues	» Code
Assigned:	Unassigned	» AlexisWilke
Priority:	Critical	» Normal
Status:	Active	» Fixed

This is obviously also different.

define('VERSION', '6.17-dev');

I'll redirect this issue to another queue: module developers should all be aware of this! It's surprising we had to find about it the hard way. Only a minority of contrib module developers are also core developers (using Drupal-dev).

Log in or register to post comments

Comment #19

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 05:25

Title:	Plenty of E_ALL notices	» Plenty of E_ALL notices in modules developed on installation with Drupal tarball
Project:	Table of Contents	» Documentation
Version:	6.x-3.x-dev	»
Component:	Code	» Other documentation issues
Assigned:	AlexisWilke	» Unassigned
Priority:	Normal	» Critical
Status:	Fixed	» Active

Many contrib module developers only develop on a drupal installation using the latest tarball from the current stable version.
All these developers miss on plenty of E_ALL notices.

Log in or register to post comments

Comment #20

jennifer.chang CreditAttribution: jennifer.chang commented 11 April 2010 at 05:27

What I mean is: a way should be found to notify such developers (current and future) of this issue.
They should either patch their Drupal tarball, or use the -dev version from the stable branch.

Log in or register to post comments

Comment #21

AlexisWilke CreditAttribution: AlexisWilke commented 11 April 2010 at 05:44

I updated my script to automatically remove the E_NOTICE from my development server. 8-)

I'm glad you noticed that a few other modules had the same problem. I reported those errors too to several modules.

Alexis

Log in or register to post comments

Comment #22

AlexisWilke CreditAttribution: AlexisWilke commented 11 April 2010 at 05:45

Title:	Plenty of E_ALL notices	» Plenty of E_ALL notices in modules developed on installation with Drupal tarball
Project:	Table of Contents	» Documentation
Version:	6.x-3.x-dev	»
Component:	Code	» Other documentation issues
Assigned:	AlexisWilke	» Unassigned
Priority:	Normal	» Critical
Status:	Fixed	» Active

Sorry, I messed around with the settings... Fixing those now!

Log in or register to post comments

Comment #23

AlexisWilke CreditAttribution: AlexisWilke commented 14 April 2010 at 22:22

Something to think about:

For all the programmers who are on your security list, you may want to include information about this notice (maybe write a concise Help page...) by including a link in your security report with something such as:

And please, do not forget to use E_NOTICE on your test servers see [page link].

This would eventually save time to a lot of people in regard to using the wrong variable and wondering why something or other does not work properly.

Thank you.
Alexis Wilke

Log in or register to post comments

Comment #24

AlexisWilke CreditAttribution: AlexisWilke commented 30 April 2010 at 01:39

I guess that the E_ALL & E_NOTICE are mentioned in the documentation as we can plainly see here:

http://drupal.org/node/34341

I'm wondering whether D7 could have a flag in an interface instead of a PHP flag, even if that PHP flag is expected to be in the settings.php file...

Thank you.
Alexis Wilke

P.S. Since I started using this error detection mechanism, I fixed one security bug and 4 or 5 other bugs in that many modules. I think that's a given that each programmer should know about this issue...

P.S. I did not test D7, but I can see that you can switch the error reporting level from within the Interface! So we can hope that developers will catch many of those errors.