Allow embedded images in project description

Code comment: we should wrap a check_url() around the screenshot URL (depending on the format). We should add an alt-attribute for level 1 accessibility.

What happens with projects who configured a screenshot link? The Drupal project's screenshot link, for example, points to a gallery (see http://drupal.org/project/drupal). Maybe we need to add a second URL so we can do stuff like they do at the Firefox plugin directory (see https://addons.mozilla.org/extensions/moreinfo.php?id=220&application=fi... and note the 'more previews' link below the screenshot).

Would you be able to work on this some more, so we can implement this properly? I'd be happy to help test/review such functionality and roll it out on drupal.org.

Code looks good. Not tested yet.

not having tested it, a few comments on the code itself:

1. according to http://drupal.org/node/56944, the project module's excessive use of wrapping everything in <p> tags makes for invalid HTML in some contexts. also, i've had reviewers set my patches to "needs work" because i was doing markup directly in the code (e.g. a <br/> or something to make it look better). so, i'm not sure if we want to wrap the screenshot image in a <p> like this. isn't the proper way to handle this to wrap things in <div> tags so they can be spaced/layed out nicely in the theme layer?
2. "Screenshot image" (as both the end-user visible field name, and the field in the DB) seems a little more natural/obvious to me than "Preview URL". "preview" seems to have special meaning (and is pretty overloaded, as it is), whereas "screenshot" is a pretty wide-spread, well-understood term. i suppose people could use this field for things other than screenshots, if they really wanted to (e.g. a project's logo, etc). but then "preview" isn't any more likely to be correct, either.

   it's also confusing to me what's different about the "preview url" and the "screenshots" fields, both of which are actually urls. the difference is that the new field is automatically wrapped inside an IMG link, whereas the other is expected to be a URL to some external page with screenshots on it. since these are both URLs, the main difference is that one is treated as an inline image, whereas the other is an external link. i think the field titles should reflect that to help avoid confusion.

   so, i'd propose we either call this "screenshot image", or just "project image", if we want to keep open the possiblity that other sites might use this for something other than a screenshot.

3. why does the description say "should be 320x200"? why does it matter in this context? if we're making a single page with a bunch of these, and we wanted them to all be consistent, that'd be 1 thing. but i don't see a need to recommend a certain size if it's just going to be displayed per-project.

haven't had a chance to actually test, but i'll hold off on that until either a new patch materializes or someone tells me these concerns aren't valid. ;)

thanks,
-derek

der thanks for taking this one.

Some questions/comments:

1. On drupal.org we're already displaying theme images (through _nodeapi?). Should we use/generalize that approach instead? Associating an image with a node is a common need beyond this use.
2. Agree with dww re use of <div>instead of <p> tags and also about the use of 'screenshot' instead of 'preview'.
3. alt="preview" in patch should use t()
4. Potential concerns about linking to external images (may be unavailable, may move, etc.). For many sites - albeit not drupal.org - a simple image upload would be preferable.
5. Having a standard image size is useful for consistent display, especially when used with teasers.
6. Do we want two different sizes, one for teasers and one for full page?

the lack of this feature is creating extra work and some grief for drupal.org maintainers (http://lists.drupal.org/archives/infrastructure/2006-06/msg00161.html). bumping the prio (as a reminder for me to work on getting this committed asap).

the previous patch no longer applied cleanly, since there's already a project_update_2(). also, the previous patch's update failed on pgsql. i fixed that (and tested on both pgsql and mysql), too.

furthermore, i was very confused when first testing the patch, since no images were showing up on the project node itself. only after looking at the diff (and surrounding code) again, did i realize that the patch was only adding the image to the project summary listings. so, i added the same img div to the project node itself.

i'm still not thrilled with the 'IMG URL of a screenshot for this project.' description, since i think folks will be confused with what they should really enter. so, i'll attach a 2nd patch with a longer blurb in there.

otherwise, this i think this is RTBC. i've reviewed and tested, and it's all happy.

here's the version with the more verbose description on the image field, which reads:

URL of a screenshot for this project. Note: the value you enter here will be automatically wrapped in an IMG tag, so it should either be a full URL to an external image, or a relative path to a file that you have uploaded onto the site.

(the previous patch just has the 1st sentence, not the 2nd).

also, while i was at it, i made the alt tag say "project image" instead of "preview", and changed the name of the screenshots link to "more images" instead of "more previews"...

in discussing this patch in IRC, killes brought up a number of security concerns. his proposals are good, but are causing this issue to turn into far more work than i have time to deal with at this point. y'all should figure out if you're willing to trust people with cvs accounts enough to not worry about these things, or if you'd rather find someone to make a new version of this patch that locks the functionality down more securely. here's the transcript:

killes: well, basically we'd allow a project maintainer to point an url to any URL
Derek: indeed.
killes: so I think you should reject the url if it is suspicious
killes: and also possibly restrict the domains where it can be placed.
killes: suspicious urls are those that fail the munging test in upload module.
Derek: what's "suspicious" mean? we already do a check_url()
killes: ah
Derek: isn't that good enough?
killes: check_url is only for xss
killes: you could maybe point the url to malicious script
Derek: can't you do that already, even with filtered html?
Derek: or you're saying it's diff b/c it's an IMG
Derek: so we automatically download
Derek: instead of having to click on it.
killes: yes
Derek: hrm, this is a reasonable concern, but a) dries seemed happy with this approach (maybe he just didn't think about this stuff).
killes: I think we should do the munging test (test for double extensions) and restrict the domain to cvs.drupal.org
killes: he probably forgot
Derek: hrm, ok.
Derek: cvs.drupal.org?
Derek: oh, like viewcvs. ;)
Derek: and you have to commit your images into your project's folder
killes: yes
Derek: i'm not thrilled. then folks will automatically have to checkout these img files (which might be fairly big) everytime they checkout a copy of your project
Derek: should we enable file attachments on project nodes?
killes: it makes sense to have a screenshot in cvs.
killes: people can see what they are going to get
killes: and you need one for the them preview inside Drupal anyway
killes: well, we should check that the images aren't too big anyway.
...
Derek: well, crap, this is turning into a *lot* more work than i really have time for.
killes: dww: postpone it, then

there you have it. i'm not going to mark as postponed, but i'm unassigning myself. if someone else wants to take the ball and run with all this, great. dries, if you think it's ok to go with the existing patch, that's fine, too. but, i can't spend much more time on this right now. :(

While extra security measures would be nice to have, I don't think they are strictly necessary at this point. The only thing that is striclty necessary, is some extension checking I think (it needs to be a recognized image file type).

I believe we can trust our contributors, and that they will not abuse such functionality. If they do, we'll fix their project or block their account.

i don't know of a good way that currently exists in drupal's codebase to check the file extension of an external url. all the functions we have now (like image_get_info(), or upload_munge_filename()) really only work on files that have been uploaded. we need to check if the URL ends with an extension that's a valid image type.

at this point (and given we need this to work in 4.7.x for the time being) i propose we just write a new method in project.module to do this validation, and then we can worry about making it better/more generic and moving it into core for 4.8.

sound good?

to repeat: how about we just enable file attachments on project nodes? we'd get a lot of this crap for free, and would avoid concerns about a) what host to accept URLs to, b) what if the remote site is down/moves, etc... then, we'd just have to change the description text and validate that the given field is pointing to a file that's been attached to this node.

the problem with uploads is that there is no way to restrict people with the "upload files" permission to only upload files to projects and not say forums.

what's wrong with admin/settings/content-types/[type]? there's an "Attachments: Disabled" setting in there. we could just allow them on project_project, disable uploads entirely on project_issue, and any other node types we're worried about (e.g. forums). what types currently use uploads, which are solely restricted by permission/role?

also, back to dries's basic point about trusting our contrib community: even if we couldn't enforce this, the damage they could do by attaching a file to a forum post is certainly far less than automatically executing a malicious php script on drupal.org. worst case, they attach a yucky file, which we then delete, and warn them or block their account.

The problem is that site admins should have the ability to attach uploads to eg forums or book nodes. So we would need to allow file uploads for these node tyypes and thus for everybody.

this is getting out of scope for the issue at hand, but maybe we want an 'administer uploads' perm. ;) we could even have 2 options on each of those node-type pages, 1 for 'upload files' and 1 for 'administer uploads'. so, we could have:

type          upload     admin
--------------------------------
issues        no         no
projects      yes        yes
book          no         yes
...

this is clearly not going to happen in 4.7. but maybe some more granularity like this would be good in 4.8...

another approach for the short term: i notice as a site admin, i can create 'image' nodes, but as a regular user, i can't. site admins also get full HTML filtering privs. so, why not disable attachments for forums and books, and if site admins want to have images in there, they just create an image (which allows them to upload it onto d.o), and then in the appropriate book/form post, they just put in the IMG tag that points to the file from the image node?

but again, i appeal to the trust argument. if we trust these people to write code which we distribute on drupal.org for 1000s of sites around the world, why can't we trust them to either a) not attach files if we ask them not to or b) just let them attach files if they feel like it? they can't publish book pages themselves unless they're site admins, anyway. so, worst case, they attach something questionable to a forum. site admins already watch the forum posts like hawks for spam... ;) i really don't see the harm.

I'm in agreement with Dries. Malicious PHP in a contrib module is just as dangerous as a malicious <img> tag on a project, and the permissions are restricted (admittedly to a wide group of persons). I see no reason to do extraneous security checking beyond check_url.

I'm in favor of a solution that allows uploading of files through an attachment file. If we have the upload module enabled for site admins to use, why don't we install filemanager and attachment for project owners? This buys us a different set of permissions so we can have our cake and eat it too.

I really dislike the idea of having people enter the URL of an image hosted somewhere else. This will inevitably lead to dead images as people shut their sites down and forget about that little Drupal module they wrote a while back.

And even if nobody likes the idea of using filemanager/attachment (two fine modules, imo), then we should try to build a feature into upload/permissions in Drupal that supports our use-case here.

I'd have to -1 anything that creates reliance on additional contrib modules. These all slow the process of updating Drupal.org.

What's wrong with just enabling the upload module on project nodes and give auth users only the ability to upload images?

Hm, this may be an idea that won't work, but could, when submitting the project node, have the cvs folder be searched for the file (screenshot-drupal.org.png), and put in the project description automatically? I don't know a lot about the project module, so I'm not sure how realistic the idea is.

Scratch that, I forgot about the screenshot field (reply #1).

Can we allow file uploads to solve this, and then use form_alter to prevent uploads to locations like the forum?

Fixed on d.o via using the image_attach module from http://drupal.org/project/image. Seems to be working nicely.