image_attach_form_alter is one of those functions that has grown and grown... it's hard to follow all the different combinations of settings, permissions, and current status of the node, which all work together to decide what actually gets shown to the user.

This feature request #581174: Set the allowed number of image attachments. add to this some more, so before we do that, let's make it sane:

1. Move out stuff that can go in its own hook_form_FORM_ID_alter(). This trims down the size of the overall function!
2. Define some boolean variables to represent all the things we need to test. With easy to understand names like $may_attach_existing and $may_upload.

Sanity! :D

CommentFileSizeAuthor
#1 680644.image_attach.tidy-up-code.patch4.54 KBjoachim

Comments

joachim’s picture

joachim commented
Status: Active » Needs review
StatusFileSize
new 680644.image_attach.tidy-up-code.patch4.54 KB

I've committed some tests for Image attach. The patch passes them, BUT the tests are incomplete as I can't figure out how to test that things DON'T show in the attached images select box. (Filed #680650: Provide a means to assert the possible values in a SELECT field with simpletest.)

So this patch needs testing by humans please!

jonathan1055’s picture

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

I've tested this against the latest dev 2010-01-11 and it applies cleanly. I've tested changing the Image-attach settings on Admin Content-type, all fine, and tested the user permissions. Everything works as expected. I also read through the code changes and it all looks good, a nice tidy-up.

I noticed in one place that you have replaced count($node->iids) with $has_existing_images which is defined as !empty($node->iids), though I guess in this instance they will produce the same result.

Jonathan

joachim’s picture

joachim commented

Yup, the count() there is just to check that the node has *some* images attached.

Thanks for testing! Will commit later today.

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Fixed

#680644 by joachim: Cleaned up code in image attach's hook_form_alter().

Status: Fixed » Closed (fixed)

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