Posted by mfb on August 27, 2008 at 12:49am
| Project: | Mailhandler |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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().
| Attachment | Size |
|---|---|
| mailhandler-admin.patch | 2.21 KB |
Comments
#1
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?
#2
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..
#3
Here's a reroll with explicit defaults.
#4
I'll try to put the same values as part of
$form[...]['#default_value']and will check it.#5
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'
#6
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...
#7
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.
#8
Do you have Drupal set to display errors on the page? If not they might just be getting logged.
#9
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.
#10
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.
#11
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.
#12
sorry, didn't noticed the issue status.
#13
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.
#14
The last submitted patch, mailhandler_admin_notices.patch, failed testing.
#15
Committed, required to make tests pass for the D7 upgrade.
#16
Was a little more difficult to review implications from top of my head but i've read, patch and looks fine to me.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.