[drupal-devel] image_import.module ready for early testing
It's not done yet, but the image_import module that works with walkah's new (and wonderful!) image module is feature-complete and ready for functional testing to begin. I tested it last night by importing batches of images ranging from 1 file to 50+ files, with and without auxiliary caption files, and although there are some things I still want to improve (such as adding really "deep" clickable help links within the module, and fixing a minor menu annoyance), it is working correctly. The code is *not* yet compliant with Drupal standards for tabs and indents and such, so don't yell at me yet for that. I happen to prefer editing with tab indents rather than spaces, so I typically use a global replace to convert the code to space indents only after I'm done with it. My CVS commit notice has hit the drupal-devel list, but I'm contacting the two of you individually because I would very much value your input and testing assistance, if you have the time, since my module specifically interacts with both of yours. James, I would also like to ask a favor of you with regard to your code. I found myself having to call _image_build_derivatives() from my code, because there just wasn't any other good way to get the resizing task done. All the higher-level published functions that call that one do a security check to see if the file came from the $_FILES[] global HTTP variable, and since my import images are pre-uploaded using FTP, SCP, etc., I can't pass that security test. So what I'm asking is, would you consider making that function non-private, a supported part of the API for your module, so that I can [reasonably] rely on it in future releases? Alternatively, would you consider exposing a new public function that accepts a skeletal $node object, with attributes of type ('image'), body, teaser, uid, title, and file (all of which I can supply), but which has not yet been created as a node in the database? The function I propose would add the node, retaining any pre-supplied attributes, and would return either $nid (integer) or the $node object itself (with new attributes, especially nid, populated). In effect, this would migrate about 25 lines of code from import_image.module to image.module, and would be a hook that other importing modules could use if their image files came from a non-HTTP source. If you aren't interested in making these changes, I'll abide with what you have now, and just risk that I might have to do a rewrite in the future. But I think the image_build_derivatives() [non-private] and/or the other proposed function [which might be called image_build_node() for example) might have value to other developers. For instance, a module might someday exist that takes PDF or PS files, renders the first page as a PNG or JPG, and turns that into an image node for placement in a book hierarchy along with a download attachment of the original file. How do I know this is plausible? Well, my boss asked me just last week if such a thing were possible, because we may have a customer willing to pay me to develop it and release the code as GPL when it's done. :-) [This is by no means a certainty; we're just exploring options. The customer has an extremely large document library of PDF/PS files and they are wanting to webify access to it.] I would very much appreciate and respect your opinions, testing reports, suggestions, and constructive criticism of my module so far. I'm looking to get it polished up in the next couple of days and then move it from sandbox to contributions/modules. I've also submitted a project node for it, but experience says it may be a week or more before that gets approved (my last one was a week ago and still isn't on drupal.org yet). The code repository is at: http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/syscrusher/image_... Kind regards, Scott (drupal.org "syscrusher") -- -----------------------+------------------------------------------------------ Scott Courtney | "I don't mind Microsoft making money. I mind them scott@4th.com | having a bad operating system." -- Linus Torvalds http://4th.com/ | ("The Rebel Code," NY Times, 21 February 1999) | PGP Public Key at http://4th.com/keys/scott.pubkey
Scott Courtney said:
It's not done yet, but the image_import module that works with walkah's new (and wonderful!) image module is feature-complete and ready for functional testing to begin.
[...]
I would very much appreciate and respect your opinions, testing reports, suggestions, and constructive criticism of my module so far.
I hope this helps: - The menu for settings should be named "image_import" (or something other than "image") and should be under admin > settings, not admin. - The menu link to the settings is wrong (it function link should be "image_import_settings" not "image_import_admin"). - It would be nice to be able to create a new gallery from the upload page. - Some of the text in the settings page is confusing, especially the stuff related to paths. "Ignore %u and %U for system user" doesn't make much sense unless you read all the text, too. - The username folders under the set path should be created automatically upon installation and whenever a new user is added. -- Tim Altman
On Saturday 02 April 2005 17:17, Tim Altman wrote:
I would very much appreciate and respect your opinions, testing reports, suggestions, and constructive criticism of my module so far.
Hi, Tim! Thanks for the test report. Comments below. (By the way, I just committed a new version with extensive enhancements into CVS.)
I hope this helps:
- The menu for settings should be named "image_import" (or something other than "image") and should be under admin > settings, not admin.
It is... admin/settings/image_import is the Drupal path. The original settings page for walkah's image module is unaffected by my new code, so perhaps you were in the wrong location.
- The menu link to the settings is wrong (it function link should be "image_import_settings" not "image_import_admin").
Okay, I'll fix that. Thanks for the bug report.
- It would be nice to be able to create a new gallery from the upload page.
This is possible *if* you set up your vocabularies correctly. The image module (walkah's version) auto-creates its gallery vocabulary if it doesn't find one upon first use. That automatically-created gallery does not by default allow free-tagging. If you manually create the gallery vocabulary before installing image.module and image_import.module, or if you manually set image.module's auto-created vocabulary to allow free-tagging (I believe it's just a matter of toggling one boolean field in the {vocabulary} table, but don't quote me on that), then you can create new galleries from the upload page. IMO, this is one of the niftiest uses for Morbus Iff's taxonomy patch! :-) Is there a way to allow similar functionality for non-free-tagging vocabularies? I'll check into it. I agree that it would be a useful feature if it's practical to implement.
- Some of the text in the settings page is confusing, especially the stuff related to paths. "Ignore %u and %U for system user" doesn't make much sense unless you read all the text, too.
Fair enough...I'll revisit the help text. The version I just published has some improvements, plus an entirely separate standalone page of instructions, but I'll bet I haven't addressed your concerns yet. If you have a moment, please email me off-list to describe any other parts of this that you found confusingly-worded. (I don't want to tie up the list with that much detail.)
- The username folders under the set path should be created automatically upon installation and whenever a new user is added.
Good point, and something I had meant to do but forgot to add. I'll put it in the TODO list for a future release. Not all users should have these directories, however -- just those who have permission to mass-import images using this module. The idea is a good one, but it's not quite as simple as just creating a directory. What I probably need to do is to create it only when the user actually comes into the image_import main page for the first time. There are also some security problems possible (too tight, not too loose), because the default permissions on new directories created by the web server often don't allow ordinary users to read or write within the new directory. But I'm hesitant to just blithely open 777 permissions on a directory that is auto-created. That might rankle some sysadmins (I know it would not please me, for one). I'll give this some serious thought, though. If you have a patch, please feel free to send it to me. I would appreciate people contacting me before doing a CVS code commit of subtantial size during this period of active coding. Thanks again for the test report. I'd be very interested to hear what you think of the new version, even though it was committed before I saw your note and therefore doesn't have some of your bug reports fixed. By the way, you didn't mention one particular test case: Did the darned thing actually work in your environment? {GRIN} Kind regards, Scott -- -----------------------+------------------------------------------------------ Scott Courtney | "I don't mind Microsoft making money. I mind them scott@4th.com | having a bad operating system." -- Linus Torvalds http://4th.com/ | ("The Rebel Code," NY Times, 21 February 1999) | PGP Public Key at http://4th.com/keys/scott.pubkey
Scott Courtney said:
On Saturday 02 April 2005 17:17, Tim Altman wrote:
I would very much appreciate and respect your opinions, testing reports, suggestions, and constructive criticism of my module so far.
Hi, Tim! Thanks for the test report. Comments below. (By the way, I just committed a new version with extensive enhancements into CVS.)
OK, I'll check it out tomorrow. [...]
By the way, you didn't mention one particular test case: Did the darned thing actually work in your environment? {GRIN}
No, but I'm not sure if it was due to your module or something else. :) I told it to upload 15 images. After waiting a bit, I went back to find a blank page. I was able to access admin settings and found that only four images had been created. However, the main page of the site was just blank. I tried deleting the imported images, but the main page didn't come back until after I disabled image.module. To be honest, I hadn't checked the main page before importing, so it could have been that way already. I'll look into it a bit more tomorrow. I'll reply to the rest of your e-mail tomorrow. -- Tim Altman
On Sun, 03 Apr 2005 00:32:46 +0200, Scott Courtney <scott@4th.com> wrote:
On Saturday 02 April 2005 17:17, Tim Altman wrote:
I would very much appreciate and respect your opinions, testing reports, suggestions, and constructive criticism of my module so far.
[...]
- It would be nice to be able to create a new gallery from the upload page.
[...]
Is there a way to allow similar functionality for non-free-tagging vocabularies? I'll check into it. I agree that it would be a useful feature if it's practical to implement.
I don't have the free-tagging patch installed, so that's what I was referring to anyway. :) [...]
- The username folders under the set path should be created automatically upon installation and whenever a new user is added.
Good point, and something I had meant to do but forgot to add. I'll put it in the TODO list for a future release. Not all users should have these directories, however -- just those who have permission to mass-import images using this module.
Agreed. I thought of writing that originally.
The idea is a good one, but it's not quite as simple as just creating a directory. What I probably need to do is to create it only when the user actually comes into the image_import main page for the first time. There are also some security problems possible (too tight, not too loose), because the default permissions on new directories created by the web server often don't allow ordinary users to read or write within the new directory. But I'm hesitant to just blithely open 777 permissions on a directory that is auto-created. That might rankle some sysadmins (I know it would not please me, for one).
I think similar problems have been encountered with creating the 'files' directory when an admin first visits admin > settings. Maybe Drupal should have a generic 'create directory' utility function that handles all the permissions automagically. I didn't get a chance to test your updates this weekend. Hopefully I'll have some time this coming week. -- Tim Altman
On Monday 04 April 2005 03:25, Tim Altman wrote:
On Sun, 03 Apr 2005 00:32:46 +0200, Scott Courtney <scott@4th.com> wrote:
The idea is a good one, but it's not quite as simple as just creating a directory. What I probably need to do is to create it only when the user actually comes into the image_import main page for the first time. There are also some security problems possible (too tight, not too loose), because the default permissions on new directories created by the web server often don't allow ordinary users to read or write within the new directory. But I'm hesitant to just blithely open 777 permissions on a directory that is auto-created. That might rankle some sysadmins (I know it would not please me, for one).
I think similar problems have been encountered with creating the 'files' directory when an admin first visits admin > settings. Maybe Drupal should have a generic 'create directory' utility function that handles all the permissions automagically.
Perhaps so...but it's not something I have time to do right now. :-) If you want to take a crack at a patch to auto-create the directories only when it makes sense to do so, I would certainly consider it for the next version. I'm trying to avoid having people commit CVS on this module without letting me review the patches first, but I would be glad review and probably accept a well-coded patch of this type.
I didn't get a chance to test your updates this weekend. Hopefully I'll have some time this coming week.
That's okay. We've had a death in my wife's family and I don't think I'll have a lot of time to code this week anyway, so don't sweat it. (And please, if anyone else submits test reports this week, don't be offended if I don't respond right away. My first priority is to be there for my wife, as I'm sure you understand.) Scott -- -----------------------+------------------------------------------------------ Scott Courtney | "I don't mind Microsoft making money. I mind them scott@4th.com | having a bad operating system." -- Linus Torvalds http://4th.com/ | ("The Rebel Code," NY Times, 21 February 1999) | PGP Public Key at http://4th.com/keys/scott.pubkey
On Saturday 02 April 2005 17:17, Tim Altman wrote:
I would very much appreciate and respect your opinions, testing reports, suggestions, and constructive criticism of my module so far.
I hope this helps:
- The menu for settings should be named "image_import" (or something other than "image") and should be under admin > settings, not admin.
Fixed, will be committed to CVS later tonight.
- The menu link to the settings is wrong (it function link should be "image_import_settings" not "image_import_admin").
Fixed, will be committed to CVS later tonight. Good catch on both of these, by the way. I didn't see these!
- It would be nice to be able to create a new gallery from the upload page.
There doesn't appear to be an easy way to do this, except for the free-tag vocabularies. Incidentally, I have successfully tested that you *can* manually use SQL to mark the default image vocabulary (created by image.module) to be a free-tag vocabulary, and it seems to work correctly afterward.
- Some of the text in the settings page is confusing, especially the stuff related to paths. "Ignore %u and %U for system user" doesn't make much sense unless you read all the text, too.
I've done a general reorg/cleanup of those prompts in the version I'm about to commit to CVS. :-)
- The username folders under the set path should be created automatically upon installation and whenever a new user is added.
See my previous note to the list on this. It's non-trivial, although I admit it's a good idea in principle. Scott -- -----------------------+------------------------------------------------------ Scott Courtney | "I don't mind Microsoft making money. I mind them scott@4th.com | having a bad operating system." -- Linus Torvalds http://4th.com/ | ("The Rebel Code," NY Times, 21 February 1999) | PGP Public Key at http://4th.com/keys/scott.pubkey
participants (2)
-
Scott Courtney -
Tim Altman