[D7] File Replace

Needs a link to your sandbox in order to review.

Also just want to check if you've had a chance to review some existing modules to see if you could either extend them?
http://drupal.org/project/upload_replace
http://drupal.org/project/media_update

Hi brunogoossens,

You should add those description of the module differences to the project page, to help users understand more specifically when they'd want to use your module vs. similar options.

Some code issues:

• '#description' => t(''), If there's no description, this whole line should be removed.
• t('Allowed extention: ' . $extention); Correct spelling: extension. More improtantly, you shouldn't include variables directly in t(), since that makes translation difficult/impossible. See documentation for how to use placeholders in t().
• It's not clear to me why file_replace_form_file_replace_form_alter() exists. Why not just set these values in file_replace_form()?
• unlink(drupal_realpath($old_file_uri)); This should use drupal_unlink, which apparently works better on Windows.

Please set this back to "needs work" when those issues are fixed.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/file_replace.module:
   +10: [normal] Menu item titles and descriptions should NOT be enclosed within t().
   +21: [minor] Missing parenthesis after function name
   +31: [minor] Missing parenthesis after function name
   +161: [minor] put a space between the asterisk and the comment text
  
  sites/all/modules/pareview_temp/test_candidate/views/list_files.inc:
   +-1: [normal] @file block missing
   +4: [minor] Missing period
  
  Status Messages:
   Coder found 1 projects, 2 files, 2 normal warnings, 4 minor warnings, 0 warnings were flagged to be ignored
  

• README.txt is missing, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• Comments: there should be a space after "//".
  

  file_replace.module:107:        //Remove old file
  file_replace.module:114:      //update all nodes, This is nessesary for caching and other node change reactions.
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/views/list_files.inc:
   +10: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 2 files, 1 minor warnings, 0 warnings were flagged to be ignored
  

• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• ./file_replace.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function file_replace_views_api() {
  --
  
  function file_replace_permission() {
  --
  
  function file_replace_page($fid) {
  --
  
  function file_replace_form() {
  --
  
  function file_replace_form_submit($form, &$form_state) {
  --
  
  function file_replace_form_file_replace_form_alter(&$form, &$form_state, $form_id) {
  --
  
  function file_replace_attached_content($fid) {
  

• ./views/list_files.inc: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function file_replace_views_default_views() {
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/file_replace.module:
   +28: [minor] Missing period
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 minor warnings, 0 warnings were flagged to be ignored
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...p709/sites/all/modules/pareview_temp/test_candidate/file_replace.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AND 3 WARNING(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    7 | ERROR   | "require_once" is a statement not a function; no parentheses
      |         | are required
   10 | WARNING | Format should be * Implements hook_foo().
   28 | WARNING | Format should be * Implements hook_foo().
   57 | WARNING | Line exceeds 80 characters; contains 92 characters
  --------------------------------------------------------------------------------
  
  
  FILE: ...709/sites/all/modules/pareview_temp/test_candidate/views/list_files.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   15 | ERROR | Calling class constructors must always include parentheses
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual additions:

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.

HTH,dave

Hi Bruno

I did a a review and saw these problems:
- Replace the require_once to load your inc file with drupal_module_include
- Why do you pass the $fid in function file_replace_page($fid)? As you're not doing anything with $fid.
- Your master branch still contains files other than a README.txt, and there's no README.txt in the master branch: follow step 5 on this page
- Each file must contain only one blank line, you have two blank lines at the end of each file.
- This is bad practice:
t('Allowed extention') . ': ' . $extention;
Please use placeholders! This should be changed to:

t('Allowed extension: @extension', array('@extension' => $extension))

By doing this, $extension will be sanitized. As this is user input, and could possibly be dangerous and lead to XSS problems.
- The select query should become a dynamic query. I see that you're looking into it. So, I don't see this so much as a problem.
- In file_replace_attached_content($fid) the return statements are way too long. The number of characters on a line must be limited to 80 for readability purposes.

http://drupal.org/project/coder_tough_love can help as well. Didn't realize this was such a strict process for something like line length, esp since tough love says 80 line length is for comments

Not every issue mentioned here is a blocking issue. The non-blocking issues, such as line length, are mentioned to help new contributors understand community norms, which aren't always requirements to follow, just helpful. We're not always as clear as we could be on that distinction.

This patch fixes all in #11, except for the select query being transformed to a dynamic query, which I believe should not be a blocking issue. I don't believe I have permission to push changes here, so I'm attaching the patch instead. It should be a trivial matter to remove all the files from the master branch, with the exception of a README.txt as @phoenix linked to. I'm new to contributing so if I did something stupid, I'm sure someone will let me know.

@brunogoossens As we discussed I also have some other changes/features that might be valuable to merge into the module, but I'd like to see this move out of the sandbox first with the basic functionality. I'm already relying on this module as a critical part of our workflow - thanks again.

Nice, seems to be working for me.
The only problem is that when changing an image, the new image is taking the old image's dimension.
I can see that these dimensions are save in an image field.

Yep, automated code review looks good. I installed and tested this and it works well for me.

I reproduced the issue mentioned in #17, though images are just one type of file, and this seems pretty minor. I would like to see it tackled, but I don't see it as a reason to hold back the module as a whole. (Having said that, I think it would be pretty easy to implement by splitting out line 173 and adding a bit of code to find the file and update those values:

  node_save(node_load($id->id));

I'm marking this as RTBC, I think the basic functionality of this module is good to go.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. "module_load_include('inc', 'file_replace', 'views/list_files');": you don't need that, see the docs on hook_views_api() on how to include files for views.
2. "extention" should be "extension", replace that everywhere in your module.
3. file_replace_page(): if you just want to display a form on your page you don't need that function, just use drupal_get_form as page callback in hook_menu().
4. file_replace_form(): Do not use check_plain() on #default_value in form elements, this is automatically sanitized by the form API. See http://drupal.org/node/28984
5. file_replace_form(): If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as '#type' => 'value'.
6. file_replace_form(): don not use arg(), use the parameter that can be passed to this function. See the documentation on hook_menu().

Sorry to bump this back to "needs work", but you need to know where to use check_plain() and where not.

Just curious, will this module integrate/play nicely with the media and file_entity modules?

Your project page is not very detailed (usage, installation, screenshot?), please have a look at the tips for a great project page, you may also use HTML-tags for better structure.
Your project page and readme may follow the same structure and content

Btw, you don't have to document the parameters and returned values of common hooks (but better more documentation than too few)

Your default view is not imported on installation

Your not checking whether the fid exists that is requested by admin/file_replace/%

I would suggest to use "admin/file_replace/%file" as path instead. The entity will be loaded as parameter directly and you don't have to care whether the file object really exists as the menu system will check it for you and return 404 if the entity does not exist

Don't use such generic variable names as $i

temporary values that should be remembered after submission of the form can simply be putted into the form without using a hidden form element (eg with $from['#old_fid'] = $fid)

Hi,

Some manual review of the code.

The file file_replace.views.inc should be renamed to file_replace.views_default.inc as you are implementing the hook_views_default_views() hook here. Please refer to to the API document of hook_views_default_views().. The second paragraph specifies this "This hook should be placed in MODULENAME.views_default.inc and it will be auto-loaded. MODULENAME.views_default.inc must be in the directory specified by the 'path' key returned by MODULENAME_views_api(), or the same directory as the .module file, if 'path' is unspecified."

Please make @file comment more descriptive, both in .module and .inc file. The @file tokendenotes that what follows on the next line is a description of what this file does. This one-line description is used so that api.module (see http://drupal.org/project/api), Drupal’s automated documentation extractor and formatter, can find out what this file does.

In the file_replace.module, line #14 to line #23

function file_replace_menu() {
  $items['admin/file_replace/%file'] = array(
    'title' => 'Replace file',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('file_replace_form',2),
    'access arguments' => array('use file replace'),
  );

  return $items;
}

It is best to avoid underscores(_) in the URL, so please change to

function file_replace_menu() {
  $items['admin/file-replace/%file'] = array(
    'title' => 'Replace file',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('file_replace_form',2),
    'access arguments' => array('use file replace'),
  );
  return $items;
}

Remove all the TODO comments.
Line #126 to #131

$ids = db_select('file_usage', 'fu')
             ->condition('fu.fid', $form['#old_fid'], '=')
             ->condition('fu.type', 'node', '=')
             ->fields('fu', array('id'))
             ->execute()
             ->fetchAll();

You could change this to

$ids = db_select('file_usage', 'fu')
             ->condition('fu.fid', $form['#old_fid'])
             ->condition('fu.type', 'node')
             ->fields('fu', array('id'))
             ->execute()
             ->fetchAll();

db_select()->condition() defaults to IN if $value is an array, and = otherwise. [The indentation is coming wrong here, don't know some how the indentation changes on it's own. In your code please keep the indentation to 2 spaces on the line after db_select]

Will do a more review of the functionality and revert back to you.

Changing the status.

Hi,

In the file_replace.views.inc. Line #295

$handler->display->display_options['path'] = 'admin/content/file_admin';

Best to avoid underscore. User "-" instead.

$handler->display->display_options['path'] = 'admin/content/file-admin';

And Line #297.

$handler->display->display_options['menu']['title'] = 'Files'

Change "Files" to something else as there is already a "Files" title which links to admin/content/file.

Would be good, in case you can change the menu path for views display to

$handler->display->display_options['path'] = 'admin/content/file/file-admin';
$handler->display->display_options['menu']['title'] = 'File Replace';

This would remove the ambiguity which is present right now. Could you make the following changes and push a fresh code, so that the community can review it again.

Issues with Module functionality. I installed this and it isn't working properly.

I'm at /admin/content/file/file-admin
- Nodes are showing up in this list of files. See attached.
- If try to replace an Image file (i'm using the built-in Article node type), I get a SQL error, from what seems to be adding these nid:// records into file_managed. That's not right.

Minor issues:
- There is still a master branch.
- Readme says there is a 'Files' tab that appears. This tab is actually called 'File Replace'

Doesn't the File Entity module now include a File Replace functionality in the dev version? see #1123570: Replace file while retaining field metadata

How is this module different?