Directory based organisational layer.

Please run spell-check and Coder review over your code, then change the status of this issue back to "needs review".

Thanks.

is it possible to define a form with enctype="multipart/form-data"?

Yes, or else file uploads would not be possible.

Great big hint: Your project title, in addition to being immodest, contains a word that is not spelled correctly.

Two words, if the reader is from US rather than UK.

Better. And there is an issue for adding multiple-file upload capability.

First: "lx_dir": what does that mean? please find a better module name. Also for the others.

• directory_based_organisational_layer.info: what is this file used for? Please remove it.
• README.txt is missing
• it is a best practice to have the core module in the repository root directory, not in a sub-directory.
• lx_dir.info has wrong line endings ('\r' but should be unix style '\n')
• info file: remove "version", this is added by drupal.org packaging automatically
• "project = "luxpaparazzi"": please change that.
• remove files like "temp" and "test.txt"
• "Luxpaparazzi (luxpaparazzi://) stream wrapper class." please fix the name.
• "Implemenation of hook_help" should be "Implements hook_help()."

No changes from #11 detected.

Last commit might be a hint.

commit a9cfaa4f386db51a0a754a2f30a330edbd0b1fba
Author: André Pletschette <X>
Date:   Fri Oct 14 14:28:04 2011 +0200

Master Branch

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.

Argh!

If you move off the master branch and no longer keep it up to date, please clean it up so that people using the original git clone command in your application don't get the bum steer from running into code form 3 days ago.

Now actually looking at the code.

useless comments. please fill them out

/**
 * @file
 *   ...
 */

A file about DOTS!

useless @params. please elaborate

/**
 * Invokes hook_odir_acl, if any implementation returns FALSE,
 * odir_control will return FALSE (deny access).
 * Otherwise if at least one answers with TRUE,
 * odir_control will return TRUE (=allow access)
 *
 * @param $rule
 * @param $odir_path
 * @param $path_is_encoded
 */

See also @return for the return value documentation.

No newline at end of file

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file.

strange url paths

most of your menu paths are odir.something. Wouldn't odir/something , odir/ajax/ahaxcallback be better? You also use mixedCase for your urls, which is not recommended. You are also mixing camelCaseNames with under_score_separated_names. Your paths are also also over the shop instead of collected together in a common area .. say like "odir/X/X" etc. seems a little haphazzard

_odir_blocks_js()

Clean this up. This is meant to be release ready code. Even has a "# TODO !" in it. Also, just found this is called from hook_block_view and it should instead be in hook_init if it's included in everything (which it currently is), OR attached to the blocks if it's only meant to show as attached to the blocks.

commented out and debugging code

This is meant to be a release .. LOTS leftovers, including fully commented out functions.

use of global .. they're everywhere!!

use drupal_static instead

global scope crap

Don't stick $path in the global scope, or anything really.

$path = drupal_get_path('module', 'odir');

what is this???? This means that this module only runs on your windows system, no one else. All this dir stuff should be based off the files directory or something similar.

define( 'odir_SYSTEM_BASE', "D:/fotoen_new"); // variable_get('odir_system_base', odir_SYSTEM_BASE)
define( 'odir_SYSTEM_TEMP', "D:/fotoen_temp"); // variable_get('odir_system_temo', odir_SYSTEM_TEMP)

defines are also always ALL_UPPER_CASE.

no idea

Here are some oddities in the code that I just can't explain.
check_plain('-- Up --')
variable_get('odir_system_base', odir_SYSTEM_BASE) . '' . $link

$link = odir_get_parent($current_path);
odir_encode(check_plain($link))

in HTML ...
"\n\n\t"

odir_file_path_encoded()

.. does not seem to do anything, and is used in different context that need different encoding. The times it's used to encode things for the URL, is wrong. You should use drupal API functions .. drupal_encode_path. Most of your encoding/decoding should be done with know apis.

theme functions

.. are exclusively for output of html. do not include logic, TABS, etc. All of this should be done by the preprocessor so that only the html with a few variables. Ideally, it should return a render array ready for drupal_render instead of raw html* so that it can be altered by other modules.
* unless you are theming the smallest possible thing, such as a single field etc.
The two functions you have as theming functions, should be returning nested render arrays, and should probably be broken up into more theme functions. All the looping through files and carrying one should have been done in preprocess. theme is for output only, not logic!

preg_match too big

This matches filename.jpeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeg just fine.

preg_match('/\.JPe*g$/i', $dir)

This matches .jpg and .jpeg

preg_match('/\.jpe?g$/i', $dir)

This combines the two tests you have

preg_match('/\.(jpe?g|orf)$/i, $dir)

PS There's more than jpg and orf files that are images ... and it should also be user configurable.

use t() params

This doesn't translate! Nor is is checked!

$content = t("$system_dir doesn't exist!");

use this instead

$content = t("@system_dir doesn't exist!", array('@system_dir' => $system_dir);

That will automatically run the variable through check_plain for you too since it's an @ var.

delete your variables

when uninstalling your module, you should delete all variables you have created.

that'll do ..

I can't keep on hunting out issues in your code. Please take this start and revisit all of your code to clean it up and make it use the best contribution it can be. I didn't even look at any of the sub-modules.

Comment more of your functions more verbosely and precisely please. Some of this stuff is impossible to figure out what it's for without jumping through 3-4 functions, all of which are not function block commented and sometimes don't even appear to do anything.

My personal favourite thing to find in code when I'm investigating if it's worth using ... die(), even if it is commented out.
//die($field_identifier . " : " . $selected_field_id);

I'm guessing this is meant to be 'needs review'. Without status changes, issues are not seen by the right people.

The automated review scripts are flagging you on quite a number of points.
Some of them can easily fix, please check the Drupal coding standards.

You can also rerun the script and check the automated review scripts result which helps you before submitting project to needs review state.

see http://ventral.org/pareview/httpgitdrupalorgsandboxluxpaparazzi1301730git

See attachment

Forgot to change status after adding review comment #22

Coder review in #22 does not include manual review. Setting back to Needs Review.

Regarding the #1410826: [META] Review bonus program, please take note of the directions.

Instructions for reviewing

• Read How to review Full Project applications and subpages.
• Read Writing secure code and subpages.
• Learn from others how they did a review, checkout closed issues that lead to a project promotion.
• You can use automated tools like pareview.sh for review but you must always do an additional manual review.
• Pick an application from the review bonus list and get started! (If it is empty pick one from the "needs review" list)
• When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

@ELC

Thanks to put focus on a "Instructions for reviewing".

But the other side is there are many small issues regarding coding standard reported in Automated Review http://ventral.org/pareview/httpgitdrupalorgsandboxluxpaparazzi1301730git such as

• Spaces must be used to indent lines; tabs are not allowed
• Whitespace found at end of line
• There must be a single space before an operator statement

Using proper configuration in editor you can avoid first two issues

please have a look at http://drupal.org/node/318 also find the attached .txt

feel free to change the status if this was a mistake.

please don't assign your application yourself, only the current reviewer should do this

You need to add the review bonus tag as described in #1410826: [META] Review bonus. That can be done in the input field above the comment body.

Thanks for your reviews, just make sure that you pick applications that did not get a review in a long time.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. Remove the .project file from the master branch.
2. info file: not need to specify php = 5.1, Drupal 7 already requires PHP 5.2.
3. odir_help(): do not pass variables to t(), always use literal strings in t() where possible. Also: why do you need $replace?
4. hook_perm() does not exist in Drupal 7.
5. odir_control(): use the @return syntax to document return values, see http://drupal.org/node/1354#functions
6. odir_init(): are you sure that you need hook_init()? It will be called on literally every page request. Add the libraries only on pages where you actually need them, or is that not possible?
7. odir_create_directory_form(): this is not a hook, this is a form building function. See http://drupal.org/node/1354#forms
8. "$form['#id'] = "odir_createdir_form";": no need to set the form id, Drupal will do that for you.
9. "'#description' => t(''),": if there is no description you should just omit this.
10. odir_set_bread_crump(): spelling: breadcrumb.
11. odir_split_path(): you should use the l() function to generate link markup. Also elsewhere.
12. "'path' => check_plain($link),": use check_url() to sanitize URLs.
13. Please create a "templates" folder and move all your template files there.
14. hook_odir_control(): you should describe what the parameters mean and what this hooks can be used for.
15. dir_ajax_directory_removal(): why do you use check_plain() here, you are not printing anything to the user? "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from http://drupal.org/node/28984 . You are filtering on input here, which is not necessary and should not be done.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...ace/drupal-7/sites/all/modules/pareview_temp/test_candidate/odir.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
   299 | ERROR | Empty calls to t() are not allowed
   303 | ERROR | Empty calls to t() are not allowed
   307 | ERROR | Empty calls to t() are not allowed
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. "default;": there should be no semicolon after the default keyword in the swtich clause, there should be a colon ":".
2. odir_ajax_directory_removal(): as long as you do not output anything to the user you don't need to use check_plain(). And all file operations are safe and should do escaping anyway. Same for odir_ajax_file_removal().
3. odir_ajax_file_removal(): $uri is printed back to the user, so you should use "@" placeholders in t() that will sanitize the argument for you.
4. Same for odir_ajax_create_directory(): only use check_plain() on output.
5. odir_ajax_directory_removal(): "$path = odir_decode(check_plain($_POST['odir_path']));": that line appears twice, why?
6. "$wrapper = NEW OdirStreamWrapper($uri);": the "new" keyword should be all lower case.
7. odir_ajax_check_file_info(): if you want to output JSON you should use drupal_json_output().
8. "print '';": this does not do anything, why do you need it?
9. "$html_error .= 'alert(\'' . t($msg) . '\');';": do not pass dynamic variables to t() where possible, as this cannot be detected by translation extraction tools. Also elsewhere.
10. "$html_error .= 'window.top.window.odir_uploadFinished("' . $result . '")';": what do you need $result for? It is always hardcoded to -20?
11. odir_upload_files_in_iframe(): first you add a text/html HTTP header and at the end you use drupal_json_output(), which will set a different content type. why?
12. odir_acces_rules(): why do you add a space " " to the t() call in drupal_set_message()?

Sorry to bump this back to needs work, but you need to know where to use check_plain() and where not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Just a small notice...

You have implemented function odir_odir_acces_rules()

/**
 * Implements hook_odir_acces_rules().
 */
function odir_odir_acces_rules() {

with a typo in acces -> access, which can cause problems.

• Regarding url(): it uses drupal_encode_path() for clean URLs or drupal_http_build_query() for non-clean URLs, so all the escaping is done for you and you don't need to worry.
• The reported CSRF errors are false positives in the coder module and can be ignored most times.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...rkspace/drupal-7/sites/all/modules/pareview_temp/test_candidate/odir.js
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
    4 | ERROR | Whitespace found at end of line
    6 | ERROR | Whitespace found at end of line
   12 | ERROR | Whitespace found at end of line
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. odir_field_node_add(): variable $user is unused, remove it.
2. _odir_filelist_details(): doc block: you should use only one space to separate words.
3. "Strict warning: Only variables should be assigned by reference in odir_block_view() (line 674 of odir.module).": Please enable PHP strict warnings in your development environment.
4. I still don't understand what this module does. I followed the instructions in the README, what's next? Should I add files to that odir directory? What is the field on the node type used for? What are the menu paths dir and odir used for? What (sub-)modules do I have to enable in order that this project works?
5. PHP Fatal error: Cannot redeclare odir_field_help() (previously declared in odir_field/odir_field.module:10) in odir_fieldblock/odir_fieldblock.module on line 14

Enabling module Directory based access control throws several warnings (noted in README.txt "Directory based access control" and "Field to directory" are actually untested.) :
Warning: preg_grep() [function.preg-grep]: Unknown modifier 'o' in drupal_find_theme_functions() (line 1145 of E:\xampp\htdocs\adlibtools\includes\theme.inc). I continued with this module disabled as it threw these warnings on the content pages also.

Using Chrome browser:

Creating and navigating directories and sub-directories worked without error when using links in the block or the content pane. Using the breadcrumb links threw returned error 403, 'access denied'. This was because the breadcrumb urls were malformed, e.g.

I am on page http://mysite/dir/test1%7C%7Ctest1sub1 and the breadcrumb for this page is http://mysite/%7C%7Ctest1%7C%7Ctest1sub1.

The breadcrumb link for the page above is http://mysite/%7C%7Ctest1 but the 'Up' url is http://mysite/dir/test1 while the link in the content pane is http://mysite/dir/%7C%7Ctest1

FILE UPLOAD:

Uploading a .txt file resulted in a white screen with the following:
{"main":"","block":"\u003Cdiv id=\u0022_odir_filelist_details_block_1\u0022 class=\u0022draggable_file\u0022 file=\u0022test1||test1sub2||README.txt\u0022\u003E\u003Ca href=\u0022\/odir\/download\/test1%7C%7Ctest1sub2%7C%7CREADME.txt\u0022\u003EREADME.txt\u003C\/a\u003E\u003C\/div\u003E","filecounter":1}

Uploading a .jpg file resulted in a white screen with the following:
{"main":"","block":"","filecounter":0}

In each case the file upload succeeded, the files were present in the correct directory.

I can't believe that such a complex module has no unit or web tests! Nor any exception handling and very little error handling. Flying without a safety net, imo! ;)

That is as far as I got for now. Interested to see how this progresses though, good luck! :)

Forgot to change status

Hi there,

I disabled, uninstalled, synced and re-enabled the module and found that my previous settings were still there... you have no uninstall() function, its not a major problem but good practice to remove variable settings when the module is uninstalled. See hook_uninstall().

The malformed links problem has been resolved, also the file upload error. I see files in the folders now! I can download text files and view image files. Only enabled the main module and Directory based image.

Interesting to compare the URLs in different browsers for filesystem path: E:\var\dbol\test1\test1sub2\test1sub2subsub1 :
Firefox: http://adlibtools.loc/dir/test1||test1sub2||test1sub2subsub1
Chrome: http://adlibtools.loc/dir/test1%7C%7Ctest1sub2%7C%7Ctest1sub2subsub1
IE9: http://adlibtools.loc/dir/test1%7C%7Ctest1sub2%7C%7Ctest1sub2subsub1
They look nice in FF but pretty messy in the others. Hardly a showstopper however.

I think this module has a lot of potential but it is very ambitious and I can't see it getting through the review process at this pace. I don't know what other people think but if I were you I would pare the whole thing down to a set of essentials and get that working well. Ditch the sub-modules that aren't finished and just concentrate on the core of your module functioning well... and build some tests! I would not recommend this module to clients as I could not guarantee the safety of their files. I would want to see tests that handle situations such as duplicates, forced overwriting, deletion, case-sensitivity and access control. Users can get a bit tetchy about their files, when bad stuff happens to them! ;)

Forgot to change status again.

I'm trying to review your module. Did this:

git clone http://git.drupal.org/sandbox/luxpaparazzi/1301730.git directory_based_organisational_layer_

....and it's all but empty. Only a .git file in there.

Perhaps you're working on it? Just putting it out there.......

Looks like they are busy restructuring the branch:

119 sec ago 7.x-1.x

http://drupalcode.org/sandbox/luxpaparazzi/1301730.git/tree/refs/heads/7...

Is there a new git url for the main module?

(Don't worry about the URL separators for now, at least they are consistent within the application)

Much better structure now imo, a bit less daunting to review. :)

When I disable the image module I get:

Notice: Trying to get property of non-object in file_get_content_headers() (line 2476 of E:\xampp\htdocs\adlibtools\includes\file.inc).

Perhaps choose a better 'default' directory than "D:/fotoen_new"? You could try the Drupal file paths set in admin/config/media/file-system instead?

Use a form_validate() function to check that the base directory actually exists and is writable. Currently it is possible to save a blank or non-existent path. You might also want to test the regex as a badly formed regex can blow up later on.

Defined ODIR_SYSTEM_TEMP but it doesn't seem to be used anywhere.

Permission to change the base directory setting could be custom.

Typo 'weigth' in menu array:

  $items['admin/config/odir'] = array(
    'title' => 'Organisational directory settings',
    'page callback'  => 'system_admin_menu_block_page',
    'weigth' => -5,

Paths could be formatted a bit better:

  $system_dir = variable_get('odir_system_base', ODIR_SYSTEM_BASE) . "/" . $dir;

  $system_dir = rtrim(variable_get('odir_system_base', ODIR_SYSTEM_BASE), DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $dir;

To avoid rtrimming the trailing separator every time consider stripping it on form_submit.

You might rename the function odir_set_breadcrump() to odir_set_breadcrumb() ;)

Try to notify the user before operations fail, e.g. I enabled the module without changing the "Base file-system directory", keeping the default which does not physically exist on my filesystem, the module appeared to create folders (no idea where, can't find them) and when I attempted to upload a file I got:

Notice: Undefined variable: html_error in odir_upload_files_in_iframe() (line 137 of E:\xampp\htdocs\adlibtools\sites\all\modules\directory_based_organisational_layer\odir.inc).

That's it from me for now! :)

... and forgot to change status again...

Reviewing....

Automated review:

see attached file in comment #64

Manual Review:

in the README.txt
typo:
Use the functionality *int* the activated blocks

I created folder in my files folder and set it up succesfuly as a root folder in
admin/config/odir/settings

I enabled blocks and created some sub folders

I tried to upload some .jpeg photos, photos were selected OK, but when I click Upload button nothing hapens.

I than manualy changed permision in new created sub folders to 777. I than clicked on one of the sub folders in drupal, but I got this:

Error
The website encountered an unexpected error. Please try again later.
Error message
Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 178 of /home/darko/workspace/drupal7/includes/entity.inc).
EntityMalformedException: Missing bundle property on entity of type file. in entity_extract_ids() (line 7501 of /home/darko/workspace/drupal7/includes/common.inc).

I tried second folder and got same error.

I tried uploading photos with block after I changed permissions to 777 but still nothing happens

The module looks good conceptualy and will be useful tool

@Darko
Thanks for your efforts, but please don't paste these ugly automated reports in here, either provide a link or just attach it as txt file.

Automated review

I create them with the module, but since upload did not work I thought it may be to do with permissions so I just 777 to test it. I'll re test .

Installation => README.txt

reviewing...

Automatic review, couple of errors, see attached file

Manual Review:
-------------------------------------------------------------------------
1.
Typo: The tile of the add folders block is:
Foldernavigation
space is missing

-------------------------------------------------------------------------
2.
I created 2 folders using the create folder block and they were created OK

However, still no luck uploading files, when I click upload , below the block I get: "Array".
Files are not uploaded.

-------------------------------------------------------------------------
3.
When I click on the folder link on the Drupal page, say DIR1, after the image upload fails, I get the message:

Directory odir://DIR1 could not be created!

However, I can see the directory on the file-system.

When I click on the same link while browsing folder, it all works fine, this only happens after the image upload attempt.

-------------------------------------------------------------------------
4.
Folder navigation works fine, I created fey folders and navigated with the block. The -- Up --
link works fine, small problem is that the up link stays there when you are in the root, I would expect it to disappear.

http://drupal.org/node/1403766#comment-5900278

reviewing

----------------------------------------------------------------------------------------------------
Uploading jpegs

I tested upload an it did work. Well done. Couple of small enhancement request:

1. When I clicked the upload button I was not sure what was happening , so I clicked it again, resulting in photos uploaded twice. So some progress bar or hourglass or something is needed to indicate that upload is happening, after user clicks the upload button.

2. Also when files are finished uploading user would expect for the folder page to refresh automatically and files to be displayed on the page without the need to refresh the page by a user. May be drupal_goto function would do the trick, where you redirect to the same page you are on after upload is finished

I also tried uploading with drag and drop in Linux with Chrome, it did work.

----------------------------------------------------------------------------------------------------
Uploading other file types

I tried uploading .png file and it uploaded ok and displayed as the link in a block, as per README.txt
However when I clicked on a link to download the file I got:
Access denied

You are not authorised to access this page.

I was logged as admin

----------------------------------------------------------------------------------------------------
Colorbox and Lightbox

I enabled both Colorbox and Lightbox modules

Than I did chose each respectively on here:

admin/config/odir/settings_odir_image

but neither worked, when I went to the page with images they were displayed as before I enabled those modules

Also can you add to the Readme file a bit about this page:
fladmin/config/odir/settings_odir_image

---------------------------------------------------
keep going, nearly there....

> Uploading jpegs
Images were placed on the page , see attachment screenshot

> Colorbox and Lightbox
no errors, just nothing hapens, see same screenshot, I was expecting lightbox or somthing to appear on the same page.
Or was something suposed to hapen?
Firefox and Crome

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

• PHP Fatal error: Cannot redeclare OdirWebTestCase::testDirectoryCreation() in ./odir_web.test on line 45
  Errors parsing ./odir_web.test

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. odir_odir_file_list_noimages_item(): empty if clauses, remove them?
2. _odir_render_file_output(): you should use "//" style comments in function bodies, see http://drupal.org/node/1354#inline
3. odir_block_view_folder_operations(): you should use Drupal.behaviors in your Javascript instead of firing jquery directly, see http://drupal.org/node/304258#drupal-behaviors and http://drupal.org/node/756722

But apart from some minor issues I think this is ready.

> Colorbox and Lightbox
i have to check that out ... as everything works at my machine
did you click on any image?

Sorry, yes it works when you click on the immage

> Uploading jpegs
i mean javascript should add those images and files directly at the end of the list without any page reload ... didn't this work for you?

Nop, nothing happens at all on the page after clicking upload button, when uploading jpegs. When I refresh page manually images do appear in the center of the page, and Drupal message appears at the top of the page
SH01-13_1.jpg has been added (b).
SH01-19_1.jpg has been added (b).

When uploading other file types the files do appear at the bottom of the block.

I cleared caches now, and it works, cool
jpeg images are uploaded on the center of the page, no refresh needed

Thanks for your contribution and your work in the application queue! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.