Topic Map Exporter

I haven't tested the module yet, just pure code review.

Attached is a patch for mostly coding standards fixes.
You should likely also run it through coder module and see what I missed.

The biggest problem is the tpl.php files, they are full of PHP logic that should be in a template_preprocess function, and are also very confusingly indented.

Thanks!

• check your email notifications at http://drupal.org/project/issues/subscribe-mail/projectapplications
• It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
• topic_map_export.module: @file doc block missing, see http://drupal.org/node/1354#files
• Have you thought about supporting non-core entities as well? You can use entity_get_info() to retrieve info about all registered entities. But as I see you do very entity-specific things in export_batch.inc, so maybe this is not an option.
• Add doc blocks to all your functions. Makes it also easier to identify single function blocks.
• topic_map_export_process_associations(): do not use "/** ... */" style comments in function bodies, use "//" there.
• class EntityEnum, class DrupalTopic: all classes need to be prefixed with your module name to avoid name clashes. Also for class DrupalAssociation
• DrupalTopic tries to handle all entity types and is therefore quite bloated. Have you considered using inheritance and one class per entity type to simplify things?

@gripster has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256#abandonedtwoweekscontact

This looks like a really nifty module! I have been trying to test it again a large site I manage, and had some success but noticed a few problems.

• First, you should update your code to reflect Drupal coding standards. The pareview tool online is really handy, and there are several issues it brings up for your module. http://ventral.org/pareview/httpgitdrupalorgsandboxgripster1189182git
• It seems the taxonomy term plugin isn't working correctly. Both times I tried to make topic maps using term references, I got this error: An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /batch?id=3207&op=do StatusText: Service unavailable (with message) ResponseText: EntityMalformedException: Missing bundle property on entity of type taxonomy_term. in entity_extract_ids() (line 7501 of /var/www/includes/common.inc).
• It's good that you are using the 7.x-1.x branch, but you should remove all files from the Master branch except for the readme.

Keep up the great work! I look forward to making some nice graphs of the larger sites that I manage.

well now http://ventral.org/pareview/httpgitdrupalorgsandboxgripster1189182git seems fine

Sorry for the long delay here :(

Your project page looks a little short, you may have a look at the tips for a great project page.

1. .module, _permission: The permission description is not very informative, either find a more useful one or you should remove the description completely.
2. 'restrict access' => TRUE,: This should only be used for permissions with high security impact(=: only admin should have it), is it really needed here?
3. If you think your module provides administrative functionality, you may move it's path from topic_map/export to eg. admin/reports/topic-map/export
4. You can also add this path as config-link by using "configure = " in your .info
5. Your invoking hooks, you got to document them in a api.php file (have a look at core on how to do it).
6. .module, _create_topic(): Seems to me like you're working around the entity_extract_ids() function
7. Globals should be defined in the beginning of the file, also you should document them with /**/ not //
8. Your switching between "" and '' very often and kind of unsystematic, maybe you could concentrate on one? Please prefer '' and use "" only when you had to escape chars otherwise.
9. Wouldn't it make sense to put strings in your module/*.inc files into t() to make them translatable ?
10. module_invoke_all('topic_map_export_association_info_alter', $associations);: If you want to invoke a hook that enables others to actually being able to alter the data you pass; you got to use drupal_alter() instead.
11. Please do your function documentation this way:
    

    /**
     * Short function description. / Implements ... ().
     *
     * Long function description giving information about eg. what,
     * why and how something is done.
     */

As pointed out, some of these are only suggestions, apply them if you feel comfortable with them ;)

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Hello,

I strongly advise you to use the link below to review your application. There are a few coding standard errors and using the tool below you will be able to make changes and verify them by yourself before submitting to the review process.

http://ventral.org/pareview

And please create a new branch and set it as the default:
http://drupal.org/node/1127732
http://drupal.org/node/1659588

Let me know if you need help with anything.

Regards,
Caio

Without having tested the module I think the code looks pretty good/complete.
I have however a few remarks:

1. In general I miss documentation for @param(s) and @return in documentation blocks throughout the code.
2. The different types of 'export types' are defined in .inc files in a directory called: 'modules'. To avoid confusion about the meaning of the directory I think would be a better call it: 'plugins' or something similar. Otherwise one might think that it contains Drupal submodules.
3. export_batch.inc, line 168: You are encouraged to make use of the Drupal API as much as possible. Is there any particular reason why you make use of fwrite instead of for instance file_save_data (http://api.drupal.org/api/drupal/includes!file.inc/group/file/7)?

Looks good to me, a quick feedback, please include decription in the implementation of hook_permission(Line#49 of .module file). Would revert back after testing the functionality of the module.

It looks good to me.

Some issues I found:

Automated test & general stuff
--------------------------------------------
> I added the url to your git repo
> Automated testing found one error: http://ventral.org/pareview/httpgitdrupalorgsandboxgripster1189182git (you should fix it, but that's not super important I guess)
> Your code does not seem very well commented, please make sure it´s clearly understandable by other developers. Please have a look at the module documentation guidelines and the Doxygen and comment formatting conventions. I think your function and general comments are really ok, however there aren't much comments inside functions, so it's really hard to read your code.

Line by line coding review
--------------------------------------------
> In your README.txt there is a wrong link on line 16
> I couldn't really check the functionality because I don't want to download 180MB wandora…, however the export-functionality and the XML-output looks fine.

I haven't found any big issues, and think your code is well written.
I'm going to change the status to RTBC

• If your including files of your own module you don't have to use drupal_get_path(); dirname(_FILE_) would do the same with less impact
• in api.php, line 90, there's the hook_ missing
• Never use 1 or 0 for boolean values -> always use TRUE or FALSE
• catch (Exception $e) { watchdog_exception('topic_map_export', $e);I don't think PHP exception strings will already be encoded properly. Maybe you better make use of check_plain() here because the $message will not be filtered before it will be displayed on the logs.
• You don't have to tunnel a variable through check_plain() or filter_xss() if you already filtered it correctly by t(). (I know automated reviews complain about this, but this one is a false positive). I highly recommend you to read the documentation about how to write secure code: http://drupal.org/writing-secure-code

But I couldn't find any major issues

Thanks for your contribution!

I updated your account to let you 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 get 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.