[D7] GPX rename

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxbalagan2163381git

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.

Hi,

You a have some error in your code and you need remove master branch.

Please see this link: http://pareview.sh/pareview/httpgitdrupalorgsandboxbalagan2163381git

Hi, you have a 2 errors, please check it : http://pareview.sh/pareview/httpgitdrupalorgsandboxbalagan2163381git

Code auto testing now seems fine.

I also installed module on fresh Drupal. Everything works as expected, but some short documentation is needed. Because only with third attempt I got waypoint names renamed. The right workflow that could be written in documentation:

1. Create file field for node.
2. Go to admin/config/gpx/gpx_rename and choose and save settings for this field.
3. Clear cache.
4. Create new node and upload test file.

Hi @balagan

You've some errors, please review this: http://pareview.sh/pareview/httpgitdrupalorgsandboxbalagan2163381git

@balagan, thankyou for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. However GPX field module exist there but looks different from this one.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(*) No: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) gpx_rename_rename_gpx(): It producing following warnings and notices because of unorganized code in this function.
   

   Notice: Undefined variable: cmt in gpx_rename_rename_gpx() (line 96 of E:\xampp\htdocs\drupal730\sites\all\modules\2163381\gpx_rename.module).
   
   Warning: date_timestamp_get() expects parameter 1 to be DateTimeInterface, boolean given in gpx_rename_rename_gpx() (line 97 of E:\xampp\htdocs\drupal730\sites\all\modules\2163381\gpx_rename.module).
   
   Warning: date_timestamp_get() expects parameter 1 to be DateTimeInterface, boolean given in gpx_rename_rename_gpx() (line 97 of E:\xampp\htdocs\drupal730\sites\all\modules\2163381\gpx_rename.module).
   

   In this code, $cmt variable value is set that not declared outside this if condition and you are using this variable outside this if statement that completely break the logic of script.

   if (isset($wpt_item->getElementsByTagName('cmt')->item(0)->nodeValue)) {
               $cmt = $wpt_item->getElementsByTagName('cmt')->item(0)->nodeValue;
             }
             else {
               drupal_set_message(t('Time is not set in the cmt element of a waypoint'));
             }

   Similar thing applied for $o variable, also variable names not convincing, you should use some meaningful name otherwise it is difficult to understand like array_multisort($t, $n, $o);. You should go through https://www.drupal.org/node/299070 again. I am unsure how PAreview can skip these.

   Personally I would suggest, you should report drupal_set_message(t('Time is not set in the cmt element of a waypoint')); these message in Drupal logs instead directly printing on screen.

   Also you are checking for valid file (File is not a valid gpx file) at later stage after $doc = new DOMDocument('1.0'); that producing following warning, if invalid file uploaded there.

   Warning: DOMDocument::load(): I/O warning : failed to load external entity "public://test%20Commands%20Useful%20for%20drupal.docx" in gpx_rename_rename_gpx() (line 80 of E:\xampp\htdocs\drupal730\sites\all\modules\2163381\gpx_rename.module).

   You should check file type first and skip further process, if file format is invalid.

2. date_create_from_format(): Why this function is required in your case, please add your comment on this, also read http://stackoverflow.com/questions/2621433/date-create-from-format-equiv... once.
3. hook_help() is missing in your module.
4. gpx_rename_install(): do not juggle with module weights as this is not reliable. Is it really required in your case, please add your comment on this.
5. gpx_rename_menu(): The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. A separate permission would be good, too.
6. (+) Instead of using date, better to use format_date().
7. (+) gpx_rename_rename_batch(): You are using count($nids) 3 times in this function, can't you store count once and use that variable further, same for count($n) in gpx_rename_rename_gpx().
8. (+) gpx_rename_add_field_form(): t() missing in at number of places like at line number 20 and 23 etc that need to be fix.
9. (+) gpx_rename_add_field_form(): Wrong html tags in following code like <P> and </div etc.
   

   $form['gpx_rename_settings'] = array(
       '#prefix' => '<div id="gpx_rename_settings"><P><B>' . $message . '</b></p><P>',
       '#markup' => $string,
       '#suffix' => '</p></div',
     );

10. Project Page: Extend this little bit more, better to go through https://www.drupal.org/node/997024 again.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Nice to see, you are doing manual reviews, getting review bonus will help speed up review process for this application :)

Thanks again for your hard work.

Major issues

• gpx_rename.admin.inc:34 Variable name should be gpx_rename_path not gpx_rename_settings.
• When a filefield instance gets deleted, its entry in gpx_rename_settings variable does not get removed.
• gpx_rename.module:238,355 Missing t() around message text "item processed."
• Incorrect function signature for gpx_rename_form_field_ui_field_edit_form_alter(). It implements hook_form_FORM_ID() and therefore does not take the third $form_id parameter. This will cause PHP notices.

Minor issues

• gpx_rename.module:117,169 These calls to drupal_set_message() should probably have "error" as the second parameter.
• Missing space after lines gpx_rename.module:178,283
• Missing @param (and some @return) documentation on gpx_rename_rename_gpx(), gpx_rename_get_nodes(), gpx_rename_rename_fields_batch(), and gpx_rename_rename_files_batch().
• Incorrect docblock comment on gpx_rename_form_field_ui_field_edit_form_alter(). Should say, "Implements hook_form_FORMID_alter()."
• Almost all functions have incorrect docblock comments; they must start with an active, present-tense third person verb. E.g., "Queries for and returns the nodes with filefields in the given bundle." instead of "EntityFieldQuery to return the nodes with filefields in the given bundle."
• Fix HTML markup of "known issues" section on the project page: Move "Known problems and limitations" out of the list markup and create separate unordered list items for each known issue.
• Consider creating an appropriate git repo tag. See What are alpha, beta releases and release candidates?.

Setting status to "needs work" only because the configuration page does not work correctly.

manual review:

1. gpx_rename_install(): as already said before: do not juggle with module weights, use hook_module_implements_alter() if you need to run before/after a specific hook implementation. Also the comment is confusing, what is gpx_parser? Is that a function or a hook implementation? The function does not exist in your module?
2. gpx_rename_node_presave(): so this module only works for nodes? Then please add a comment to the README that it does not work for other entity types that might have GPX file fields. Why can't you implement some field level hook that would apply to any file field on any entity? hook_field_attach_presave() for example?

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to heddn as he might have time to take a final look at this.

Manual review:

1. There are spelling errors in the README. i.e. rame, modul, etc.
2. Known problems: might I suggest transliteration module? I'm not sure if it will resolve all the problems, but it supposed to fix those types of issues.
3. (+) The hook_install still messes with module weights. Plus it defines variable values in variable_set(). Use hook_module_implements_alter() instead and provide default values in your variable_get() instead.
4. (+) admin.inc: Don't use extra whitespace in t() calls. It makes translation difficult. The newline should disappear.
   

       '#description' => t('The path can be relative, for example public://gpx or it can be
       an absolute system path, for example f:\xampp\htdocs\drupal\sites\default\files\gpx'),

5. hook_help: Instead of simply printing out the README.txt, I'd suggest using the suggestion @ https://www.drupal.org/node/161085 and convert the README to markdown format. It will make the formatting easier to read.
6. hook_menu: If this module is the beggining of several modules, might I suggest a new permission is in order?
7. A note about only supporting nodes is in order... but that feedback was already given.
8. (*) The current implementation of this module uses a hard coded LANGUAGE_NONE. This means it won't work on any multi-lingual site. Either place a dependency on entity.module, or use field_language(), or somehow dynamically figure out the appropriate field language. I also see calls to $node->type to get the bundle. If you make this support all entities, then I would highly suggest you depend on entity. Most sites use it, so its not a huge change. And then you can can call getBundle(). See https://www.drupal.org/documentation/entity-metadata-wrappers.
9. gpx_rename_form_field_ui_field_edit_form_alter: The "Implements" name should be FORM_ID, not FORMID. See https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
10. Are you sure you want to use field_read_field instead of field_info_field in gpx_rename_field_delete_instance()? The info function is more performant.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

I think that your only (*) item about field language does not warrant setting this back to needs work, since file fields are rarely translated this problem should not be a critical application blocker. Back to RTBC I guess?

RTBC again. I was able to confirm that file fields are almost always LANGUAGE_NONE, even when content translation is enabled. A pretty specific set configuration is required to make a file translatable. However, a note should be added into module documentation mentioning the i18n limitation.

And since that was the last blocker...

Thanks for your contribution, Nagy Zsolt!

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.

#32.2: 80 characters applies to comments. Code itself can be longer than 80 characters. In general translatable strings should be logical chunks of data. Single words don't make sense, but neither does pages of text in one long t(). I tend to recommend a paragraph is the max length.
#32.8: Using entity module will resolve the issue as well.
#32.10: Depending on how that code is run, it might need an uncached copy of field info. I'm not sure in this case if it is necessary or not.