Hey Berdir and everyone,

In testing the new fieldable Private Messages in D7, I noticed the following bug: If you try to add an "Image" as a field to Private Message then an error message is displayed and the message is not sent. Here's the error message:

    *  Notice: Trying to get property of non-object in file_field_presave()  (line 258 of /Users/benkaplan/git/drupal/modules/file/file.field.inc).
    * Notice: Undefined property: stdClass::$uri in file_save() (line 515 of /Users/benkaplan/git/drupal/includes/file.inc).

Additionally, if you first try to preview the message before sending it, you get the following notices:

    *  Notice: Undefined property: stdClass::$uri in theme_file_link()  (line 690 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in theme_file_icon() (line 725 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 767 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_map() (line 809 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in theme_file_link() (line 697 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filesize in theme_file_link() (line 697 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filename in theme_file_link() (line 703 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$uri in theme_file_link() (line 690 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in theme_file_icon() (line 725 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 767 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_map() (line 809 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in file_icon_path() (line 782 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filemime in theme_file_link() (line 697 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filesize in theme_file_link() (line 697 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined property: stdClass::$filename in theme_file_link() (line 703 of /Users/benkaplan/git/drupal/modules/file/file.module).
    * Notice: Undefined index: uri in theme_image_formatter() (line 512 of /Users/benkaplan/git/drupal/modules/image/image.field.inc).

As always, let me know if you need more testing.

Best,
Ben

CommentFileSizeAuthor
#13 submit_call_missing_and_999_other_things.patch7.05 KBberdir
#11 submit_call_missing_and_1000_other_things.patch7.26 KBberdir
#3 submit_call_missing.patch2.11 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Postponed (maintainer needs more info)

I'm not able to reproduce this. Can you do two things...?

- Make sure you are using latest HEAD. I know that I've seen similiar errors too but they are gone with my current version. There was a commit recently that affected image handling

- Try to reproduce it on nodes (You already know that one... :) )

Thanks for testing again, it's really appreciated!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Message is Not Sent if "Image" Field is Added to Message » Message is not sent when image field is left empty
Status: Postponed (maintainer needs more info) » Active

Ok, I got it, it actually fails when you have a image field but don't upload an image.

What happens behind the scene is that an image related query fails and then the transaction is rolled back.

There are two thigns here..

- First, we should return FALSE in our function when roll back an exception or probably even re-throw the exception, I guess that's cleaner.

- Second, image handling still seems to be broken. At least for non-core entites..

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new submit_call_missing.patch2.11 KB

Ah, I got it.

We're missing a call to field_attach_submit() which is responsible for removing empty values. The patch also cleans up error handling in case of an exception.

Status: Needs review » Needs work

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#3: submit_call_missing.patch queued for re-testing.

Fixed the failing tests, default theme in drupal has changed.

BenK’s picture

BenK commented

I want to test the latest patch, but I'm waiting for the results of the latest queued test. Why hasn't the test reported back yet?

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Test is green for me?

There is something strange going on with the tests on the 6.x-2.x-dev branch though. Some test slaves report 23 strange exceptions, if you see that and no other fails, that means that the patch works. I'm working on figuring out what's going on there. But D7 seems to work fine.

BenK’s picture

BenK commented

Oh, maybe I'm reading this wrong. So if you fix the tests and re-submit the same patch, it won't print a new test result in the comment in which the patch was re-queued (#5)?

I do see a green (passed) result in #3... was this failed (red) before? Is the patch in #3, therefore, ready for me to test?

Thanks for the clarification...

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There is only one line per uploaded patch file. If a re-test is requested, it turns grey and states the the patch is currently being retested (while the original result is still displayed). Once the test is finished, it switches to red/green.

So yes, Patch #3 is ready to be tested.

BenK’s picture

BenK commented
Status: Needs review » Needs work

Thanks for the info. I tested the patch in #3 and it seemed to fix the originally reported error messages. I can now send a message with or without an image field attached to the message. However, a notice has showed up when previewing a message (either one with an image attached or one without an image):

Notice: Undefined index: uri in theme_image_formatter()  (line 512 of /Users/benkaplan/git/drupal/modules/image/image.field.inc).

Note that if an image is attached, the image does not show up in the preview.

One other odd thing I noticed, too: For some reason, once I added an image field to Private Messages it's introducing a bug in the default sort order (which had been fixed by the patch at http://drupal.org/node/837740). More specifically, when I send a message that doesn't have an image, it is appearing below a message with an image at /messages (even though the message without an image is more recent).

Thoughts?

--Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new submit_call_missing_and_1000_other_things.patch7.26 KB

Ok, the patch is doing 1000 different things now, poor kittens... :)

- Fixed preview for empty and actually existing images
- Added a "Preview" title to the preview fieldset
- Cleaned up some code.
- Extend the tests a bit.

Anyway, fields integration is actually starting to work as it should I'd say

BenK’s picture

BenK commented
Status: Needs review » Needs work

Berdir,

I tried to patch in #11 and all error messages are gone and things are working pretty well. However, I did notice a couple of possible bugs:

A. If you attach an image to a message and then preview it, everything is displayed properly. But on the preview screen, if you then remove the original image, replace it with a new image, and click preview again, the new image is not displayed in the resulting preview screen (no image is displayed). If you try to send the message, the new message is attached properly, so it's just an issue with changing the image and previewing it again.

B. I just realized that the message subject line isn't appearing anywhere in the preview. Shouldn't this be displayed somewhere on the preview screen? Let me know if you want me to open a new issue for this....

C. Default sort order is improved, but still weird in some cases. If a user has only received messages (and not sent any), the default order (newest first) looks good. But once the user has both sent messages and received messages in the same mailbox the order seems random (most recent sent or received is usually somewhere in the middle of the list). Would you rather that I open a new issue for this? We tackled this on another thread, but recent patches seem to have create a new version of an old problem.

Thanks,
Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new submit_call_missing_and_999_other_things.patch7.05 KB

A This preview stuff is killing me ;). I guess this is because field caches the message and the reason why http://api.drupal.org/api/function/node_preview/7 uses an internal helper function to load the content for previewing instead of the field_attach_load() function. There is also a related core issue, see #846356-4: node_preview() calls _field_invoke_multiple(). Fixed for now, we might want to keep this issue open when the patch is commited and if the core issue is fixed, clean this up.

B: Yes, let's handle preview display in a new issue. I've also removed the added Preview title from this patch. Let's commit this to have something working and then improve it visually. That visual stuff can then also be backported to D6. I think there was an issue about displaying the subject when previewing but I can't find it anymore.

C. I can't see how fields can possibly affect the sort order. That responding stuff seems more possible so yes, let's handle that in a new issue.

It would be good to have tests for the whole image/preview stuff but I'm not in the mood to write file upload tests right now :)

BenK’s picture

BenK commented
Status: Needs review » Reviewed & tested by the community

Hey Berdir,

I tried the patch in #13 and everything is working great! This is ready to be committed as soon as you have a chance.

Thanks,
Ben

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Thanks for reporting and testing, commited to 7.x-1.x-dev!

Status: Fixed » Closed (fixed)

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