[D7]Image Node Attach

I'm very pleased to see that someone's tackling this! Do please make it known on the very very long issue about the old contrib Image module's D7 upgrade path!

One thing that came up a lot is that there's more than one way to migrate attached images -- you could keep your image nodes as nodes, and use entityreference to point to them, and then have a custom formatter; or you could lose the image node and have a simple image field (and then have to figure out a strategy for dealing with an image node that's attached to more than one content node).

function _batch_image_attach_migrate_update_http_requests() {
  $_SESSION['http_request_count']++;
}

That's not the right way to do this at all. Use the batch sandbox to store details from one batch iteration to the next, it's what it's there to do.

Though I don't know why you need this anyway:

    drupal_set_message(t('@count results processed in @requests HTTP requests.', array('@count' => count($results), '@requests' => _batch_image_attach_migrate_get_http_requests())));

It's not customary to bother with saying how many requests were needed.

At any rate, use the sandbox and all the code surrounding this will boil down to 2 lines: one to initialize and one to increment.

    foreach ($image_attach_result as $image_attach_data) {
      // Each operation is an array consisting of
      // - The function to call.
      // - An array of arguments to that function.
      $operations[] = array(
        'batch_image_attach_migrate',
        array(

That's not the best way to use BatchAPI. You should only add the operation once, and let the operation iterate over a set number of results, using the sandbox to mark how far it got each time.

Though a bigger issue is that as you've called your module 'migrate', I assumed you'd be building on top of Migrate module...

Please run PAreview.sh once. I found multiple issues in your module.

USe URL: http://pareview.sh/

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Manually reviewed your code. It seems to be changed completely from last revision. Now it makes more sense to have images as node. I am going to test it fully on a migration environment and let you know the comments.

Can we have configurable node limit that you used in batch? Currently it is hard coded to 50.

Automated Review

PAReview doesnt reported any issue in current build.

Manual Review

Tried migrating images content from one of the existing Drupal 6 site to new Drupal 7 vanilla. Worked for me.

Duplication
No. Googling for drupal does not find an existing module that does this. Though found one similar custom PHP script. A full fledge module would be helpful.

Master Branch
No.

Project page
Exists. Module name changed to make it more informative.

README.txt
Yes: Followed steps from Readme file and Help page and able to complete image attach migration.

Code long/complex enough for review
Yes

Secure code
Yes: Follows guidelines for writing secure code. (There isn't much to review, but I didn't find anything wrong with the code submitted.)

Coding style & Drupal API usage
There isn't much to review, but I didn't find anything wrong with the code submitted.

There are IMHO no blockers, so I am giving +1. Let somebody else also review this module then it can be moved to RTBC.

@er.pushpinderrana

Nice Module

There are few issue
1. line no-100, image_node_attach.module file Please also check

!empty($image_node->node_image) 

otherwise if array empty it give php warning undefined index.

2. intially assign

$link_file

to FALSE in your function image_node_attach_field_formatter_view. so you not need to check isset again and again in your foreach loop. Then only check

if($link_file){ }

3. Line no 212

theme_entity_reference_image_formatter

function. In this function $item variable may has other attributes like 'error'. so also manage rest attributes.

@er.pushpinderrana, the review process has been backed up, but some new admins have come on and we are working on clearing this out as quickly as we can (without compromising the review process). Unfortunately, this is a new application and older projects have priority. There are currently four RTBC applications before you, so it will be a few days before we can take a look at this.

add a tag

Removing "PAReview: Single project promote" tag as by mistaken added. Everything is looking good to me, it is ready to go.

Git default branch is not set, see the documentation on setting a default branch.

manual review:

1. Project page: "There was a module in Drupal 6 that are deprecated in Drupal 7". Which one is it? Please provide a link.
2. "'access arguments' => array('administer access'),": that permission is not defined by your module in hook_permission()? Also the permission name does not really fit? Shouldn't it be "administer image node migration" or something similar?
3. image_node_attach_field_formatter_view(): the switch() statement does not make sense here since you only have one case? Use if() instead?
4. "'#default_value' => variable_get('image_node_attach_content_type_names'),": that variable is never used or set anywhere else, why do you need it? Please add a comment.

The misleading and undefined permission is a blocker right now, otherwise this would seem good to go.

1) PAReview is clear.

2) I did a manual review of this project as well. I also installed your module, and test functionally, it works fine. I can see all the points are fixed mentioned by @klausi.

It looks RTBC to me but I am not the right person that makes the final decision. Someone from git administrator look into this and will take a final call on this.

This looks good to me now after a manual review, so ...

Thanks for your contribution, er.pushpinderrana!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.