The mailbox add/edit form has some PHP notices about undefined indexes and undefined variables. The attached patch fixes the issue.

Part of the problem is missing default values for the mailbox form. I took a shortcut and used schema API to grab some defaults, but alternatively some explicit defaults could be defined.

Btw another thing I was looking at but didn't patch is the submit function for this form could probably use drupal_write_record().

CommentFileSizeAuthor
#13 mailhandler_admin_notices.patch2.59 KBilo
#9 mailhandler-fixes.patch6.78 KBeMPee584
#3 mailhandler-admin.patch2.14 KBmfb
mailhandler-admin.patch2.21 KBmfb

Comments

z.stolar’s picture

z.stolar commented
Status: Needs review » Needs work

The patch failed for me. Please re-roll.
Also, you might want to check out the 'cron' and 'security' radio buttons, which are stored as text, and so they are initialized to an empty string, while there should be something there.

Why did you choose to initialize the values through the DB, instead of setting them as part of the form?

mfb’s picture

mfb
they or he
commented

I just didn't know what the defaults should be (aside from what the schema defines). I can reroll it if/when I have more time to look at correct form defaults..

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new mailhandler-admin.patch2.14 KB

Here's a reroll with explicit defaults.

z.stolar’s picture

z.stolar commented

I'll try to put the same values as part of $form[...]['#default_value'] and will check it.

z.stolar’s picture

z.stolar commented

Hi Mark,

I tried to recreate the notices, but I don't see anything.
I'm using PHP Version 5.2.4-2ubuntu5.3 with apache2, and PHP 'error_reporting = E_ALL'

mfb’s picture

mfb
they or he
commented

You may need to be using the dev version of Drupal to see the notices? See http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/common.inc?r1=1.7...

z.stolar’s picture

z.stolar commented

I tried again, I changed common.inc as necessary, but still no errors.
hmmm... I must be missing something, but I don't see what it is.

mfb’s picture

mfb
they or he
commented

Do you have Drupal set to display errors on the page? If not they might just be getting logged.

eMPee584’s picture

eMPee584 commented
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mailhandler-fixes.patch6.78 KB

z put this into you index.php at the top:

error_reporting(E_ALL);
ini_set('display_errors', TRUE);
ini_set('display_startup_errors', TRUE);

And you'll see loads of errors. Here are some more fixes, the patch includes the previous ones.

z.stolar’s picture

z.stolar commented
Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs review

eMPee584, you can't RTBC your own patch :-)... but I appreciate the effort!
It took me a while to get to this round of bug fixing, but I'm here at last.

Meanwhile I applied Mark's patch at #3, with few corrections. I would like to better inspect your proposed changes, eMPee584.

ilo’s picture

ilo commented

I tried to submit a patch, but I'm unable to do so untill #1038910: Wrong endline marker in mailhandler.admin.inc file. is fixed for me. In the meantime, the testcase at #1038922: Verify basic mailbox operations in simpletest testcase. will still show the errors in the administrator pages, however the test does not insert nodes or comments yet, and some of them are still hidden.

ilo’s picture

ilo commented
Status: Needs review » Needs work

sorry, didn't noticed the issue status.

ilo’s picture

ilo commented
Status: Needs work » Needs review
StatusFileSize
new mailhandler_admin_notices.patch2.59 KB

The test will fail because of #1038928: Default encoding not being shown in system settings form.. There is a patch for this errors in the queue. This patch fixes the admin interface notices (exceptions in the test execution). There are still for fixes but they require much more rewrite than this. So the expected result here is 0 notices and 2 errors.

Status: Needs review » Needs work

The last submitted patch, mailhandler_admin_notices.patch, failed testing.

ilo’s picture

ilo commented
Title: Fix some PHP notices on mailbox edit form » Fix some PHP notices on mailbox administrative interface.
Status: Needs work » Fixed

Committed, required to make tests pass for the D7 upgrade.

cor3huis’s picture

cor3huis commented

Was a little more difficult to review implications from top of my head but i've read, patch and looks fine to me.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.