Peritus [peritus]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

I like the idea. So much that I want to try it.
But first:
Documentation: pretty good.

1* Code style: Needs the normal amount of trivial tidy-up: (from coder.module)

It would be best if you fix that on a first module for review.

2*

include_once dirname(__FILE__) . '/pdf_to_imagefield_file.inc';

Nowadays we use module_load_include()

3*

function pdf_to_imagefield_init() {
  // If FileField is not available, immediately disable ImageField.
...

hook_init() runs for every page in the site, every time. Not a great place to check for something that might just once ever be needed. Could you move this check into somewhere better, like an admin page or hook_requirements()?

/**
 * Implementation of CCK's hook_widget_settings($op = 'validate').
 */
function pdf_to_imagefield_widget_settings_validate($widget) {
	// Check that set density is valid.
  if (!empty($widget['density']) && !preg_match('/^[0-9]+x[0-9]+$/', $widget['density'])) { 
    form_set_error($resolution, t('Please specify a density in the format XxY (e.g. 120x120).')); 
  } 
}

Looks like a code error there - $resolution is undefined. (not a code review thing, just a possible bug and PHP notice)

Good Drupal code by creating a widget for your special field. Generally fits with the Drupal way well. I didn't see much code that should have been Drupal-ed more. Builds on top of imageAPI/Imagemagick, despite it being a bit awkward. It does access an ImageAPI_ImageMagick _private function - but the reason for that is documented. ImageAPI needs to be prodded like that a bit still. Using that func is still better than reinventing an equivalent one.

I tried it out! Basic UI (content type & field config) seemed to work pretty well, with adequate validation and error-prevention. I could figure out what was required from your description, but a 4-step simple 'Setup/Install' guide in the README as well would be handy. No need for "download and enable" steps, but saying that you have to create the unlimited imagefield before the pdf field would be nice.

4* Module did not work for me at first run, and the cron message didn't suggest what could be done about it.

No pdf previews generated to the book node from emerald.pdf file. But it is still in cron list.
(In my case it seems my imagemagick path hadn't been configured yet - a suggestion to check the imagemagick configs would be appropriate in case of an error)

  if (0 != _imageapi_imagemagick_convert_exec($command, $output, $errors)) {
    // Suggest you actually log an error here?
    return FALSE;
  }

I then enabled some debug, got a bit further and encountered

ImageMagick reported an error: sh: gs: command not found convert: Postscript delegate failed 

... Which I eventually solved by Setting the path to gs in the imagemagick delegates.xml file because I couldn't set my server $PATH.

And then it worked Most excellently!.

So installing isn't 100% smooth, though I know that's out of your control - we just need more error-checking and troubleshooting clues. 4*

Suggestion A note to the user explaining why the PDF isn't immediately converted, and maybe a link to cron or an option to update immediately using batch API would be really nice.

Anyway, that was a full module test, not just a code review...

Security uses normal file uploads through Drupal correctly. It sanitizes and quotes the filenames that will be used on the commandline for conversion. Uses existing Drupal roles etc to control access.

If you can address #1, #2, #3, #4 above, I'd be really happy to see this really useful module in use!

X-post removed tags, sorry.
FYI, Folk have been Wanting this sort of functionality for a while now. I don't think there is anything else in this space.

#2 Yeah, you'd see that code in earlier modules. It works, but this is a new part of the API.

#3 delete it totally. If it's from imagefield it's only there for imagefields benefit and would run anyway. I thought it was for some special case that you had encountered. And no, I wouldn't think you could disable that module by accident.

#4 just need some "what do I do now" text. Tell user to check imageapi_imagmagick, (how to) check dependencies on the server, see the README. what does something being left in the cron list mean? For me it didn't seem to mean it would try again so..?

Hi, I'm co-developer of the module.

dman, first, thank you for interesting in this and for quick and extensive replies.

#4 makes me think we should do some extra tests either on widget configuration page or on module's settings page.

1) Test if "-density" will work on a server. I have a setup atm, where specification of -density breakes everything - convert generates crappy JPG file while Drupal with watchdog keep silence. Maybe some version check? Yet to find out, i'm working on this.

2) Test that issue with delegates.xml

The normal case with convert and gs would be debugging on a case-by-case basis. If you have ghostscript installed, it should be available in the $PATH. In my case it was for users, but not for the Apache daemon. It's fixable if you are able to restart the machine and have root etc.
But fixing it is not your problem, just testing if gs works at all would help installation - is all I'm saying.
These things are normal when you require other libraries on random servers. All that's expected is making it a bit easier to find where the problem is. I just documented my travails as a part of that!

Thanks, those are great ideas. An actual test run even.
I'm really liking this!
Lets get CVS on so we can start supporting it!
Due diligence has been done on the CVS application.

And it's a kick-ass feature!

1. The license file must be removed as it cannot be committed in Drupal.org repository.

2.     form_set_error($resolution, t('Please specify a density in the format XxY (e.g. 120x120).')); 
   

   The code is accessing a not defined variable; this would cause the error message to not appear associated with a form field, and the user could not understand which form field contains the not correct value.

3.   $filefield_settings = module_invoke('filefield', 'widget_settings', 'save', $widget);
   

   Does one of the modules from which the proposed module depends require filefield.module? If filefield.module is not required, then the line after the one I reported will raise an error.

4. 
   module_load_include('inc', 'pdf_to_imagefield', 'pdf_to_imagefield_file');
   module_load_include('inc', 'pdf_to_imagefield', 'pdf_to_imagefield_widget');
   
   

   It is better to merge the include files into the module file, if they are always being included. To split the code into different files make sense when the files are included in some specific moments, as it happens for the files containing menu callback, which are automatically included from Drupal when the specific path is visited.

future TODO:

* GS (Ghostscript) version check. For example, having problem with version 8.15.x - couldn't parse latest .PDFs, created in Adobe Acrobat 9 Pro. Anyway running software released 5 years ago is not very good idea. (GS 8.15.x was released in 2004)

* An option to generate images right now, upon node save. This could be a checkbox, say "[ ] Generate now", turned on/off by default according to the module settings. New access permission for this could be added too.

* Option to choose target format. Currently JPEGs are created, while in certain cases PNG images might be preferable or required.

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

does it realy work?

http://drupal.org/project/pdf_to_imagefield