MuseScore

Hi dresden,

first of all there are quite a few issues to sort out such as indentation, whitespace and incorrect comment formats.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxdresden1856226git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Manual Review :

1) Use variable_del() to remove your custom variable like: musescore_url_ & musescore_title_, musescore_num .

2) Please add hook_uninstall() in .install file to remove custom variable.

3) Use L() function to make your links in line no : 56, 122 in module file.

4) Use t() function in line no: 56 in module file.

5) Please add README.TXT file which tell about module's description, configuration, help, etc.

6) Please do not write '?>' syntax at the end of module & install file.

A few comments:

Since this is a block, perhaps it would make sense to set the URLs on the block config form instead of having a special admin section.

Your template file does not contain any markup, just php. Instead of trying to create your own template, I think it would be better to just run the code from your template in the musescore_scorepage page callback. Just return the final output from there & Drupal will handle the rest.

Is it really necessary to run parse_url($musescore_url); & check for the host since you're setting the value in the code a couple lines up & it will always include a host?

It might be a good idea to add some error handling to check the response. If there is a problem with the url the user enters, you may end up with $musescore_data->html being undefined. I was frustrated when I got the warning (a user on a properly configured production server would probably just see a blank page & be even more frustrated). I think some kind of helpful error message would go a long way.

Changing status to needs work

Hi dresden,

Thanks very much for contributing this nice module, fixing the coding standards errors and taking into consideration previously raised suggestions.
Being myself a musician and fervent supporter of Open Source, I certainly think this is a very good initiative.

Therefore, I have spent some time to try to give a good and precise review of this module.
Please find below, a few recommendations that should help improving further this module:

1 - Major problem with variable: musescore_num
It seems the variable musescore_num is being set at installation in: musescore.install, line 12 in function musescore_install and that's the only place where it's being set.
It seems this variable is being read upon display of the block: musescore.module, line 33 in function musescore_block_view and that's the only place it's being read.

The problems here are the following:


a. What's the point in having a variable saved/recorded if it can't be changed in the backend?
Possible answers:
- No need to change it from the backend: in this case, it should be removed from the code and you could work with a constant variable

define('MUSESCORE_NUM', 5);

Which would make things easier for developers to change it, rather than having to tap into the variables table (awkward state as it is right now).
- Need to change it from the back-end (preferred solution): in this case I would recommend adding a field on the block configuration form, so in the function musescore_block_configure.
I would recommend the following code:

    $form['settings']['musescore_num'] = array(
      '#type' => 'textfield',
      [... To be written ...]
    );

The fact that it's in the settings fieldset, will display the field above/outside of the fieldset wrappers of the vertical tabs (at the bottom of the form).
Make sure you also add the validation and variable_set (saving) for this musescore_num variable in the appropriate functions, that should also be somewhere in your code.


b. musescore.module, line 61 in function musescore_block_configure:

  $num = 10;

What's the point of having a musescore_num variable, once again, if you're hard coding this?
Depending on what you choose to answer point a, I strongly recommend you consider changing this part of the code as well.

Right now, all this is buggy, the obvious issue would be that I can enter 10 different items, but only 5 will display, see:
musescore.module, line 33 in function musescore_block_view

  $num = variable_get('musescore_num');

If you were to choose to implement a backend field to configure musescore_num, it would have a great impact on the module's feature by allowing users to easily add/remove more or less fields on their block form. It wouldn't be as ideal as an AJAX add field button (similar to the core Field module), but at least it would allow adding more or less fields after save and after editing again the number of fields needed would be there, on the form. I think I've seen modules in the past do it this way.


2 - Remove path key from $form array in musescore_block_configure
In musescore.module, line 51 in function musescore_block_configure, would there be any reason why you would need the path key in $form array?
$form['musescore']['path']
Couldn't you simply work with: $form['musescore'] ?
Therefore, I would strongly recommend you remove the 'path' keys entirely from all the form items.
So for example, $form['musescore']['path']['musescore_url_' . $i] , would become:
$form['musescore']['musescore_url_' . $i]


3 - Merge all the variables references into a serialized array
Still related with the variables: musescore_url_% and musescore_title_%, this is just a recommendation, but wouldn't it be better to simply save the data in one single variable for each which would be a serialized array?
There would then be: musescore_url and musescore_title as variables then the index/key of each item in the array would allow making a correspondance between two associated values.
This would allow to greatly reduce the amount of variables in the database and since you don't seem to really need to be searching or pulling different variables (it's not like you need to access each variable independantly), I was wondering whether it wouldn't be easier (altogether) to exploit arrays instead.


4 - Simplify the CSS and make it dynamic
musescore.css, lines 1 and 17, rules such as: .form-item-musescore-title-1 and .form-item-musescore-url-1 are really weak, not clean and therefore not satisfying.
Especially if you were to think to implement a dynamic number of items, with the musescore_num variable (as mentioned above in point 1).
If you want to tweak/customize the layout of the forms, why not adding #prefix and #suffix properties to the fields?
For example, in musescore.module, line 85 in function musescore_block_configure

+      '#prefix' => '<div class="musescore-url-field">',
+      '#suffix' => "</div>",

Same for the title field, which would allow greatly simplifying the CSS rules to:

.musescore-url-field {
  float: left;
}

In this case, the style would be independant of the number of items.


5 - Maybe try to stick with forms
I'm not entirely convinced that musescore_block_save would be really needed here:
Since form validation hook callback is already implemented (musescore.module, line 118 in function musescore_block_configure_validate), why not simply implementing a hook_form_submit callback? For example: musescore_block_configure_submit.
I don't think it's written anywhere that you shouldn't be doing it this way, but why not try keeping a consistency between all your hooks, focused on forms:
a. configure [to add your fields], musescore.module, line 47 in function musescore_block_configure
b. validate [to validate the form], musescore.module, line 118 in function musescore_block_configure_validate
c. submit [to save the values, in this case: variable_set], which is currently musescore_block_save.

The rest of the logic is just related with display.

6 - Watchout for the variables to be deleted upon unistall
musescore.module, lines 98, 99 in function musescore_block_save
It seems the variables being saved currently are:

      variable_set('musescore_url_' . $i, $edit['musescore_url_' . $i]);
      variable_set('musescore_title_' . $i, $edit['musescore_title_' . $i]);

so they would pretty much be in the form: musescore_url_1, musescore_title_1.
However, in the file musescore.install, lines 19, 20, 21 in function musescore_uninstall, it seems not all the variables would be removed upon uninstall:

function musescore_uninstall() {
  variable_del('musescore_num');
  variable_del('musescore_title');
  variable_del('musescore_url');
}

I would therefore recommend you adjust these calls to match with the points above as well.
If you were to make the modifications above then (points 1 and 3, in particular), I would assume this code would be fine. Currently, it doesn't seem to be matching with the rest of the code.


7 - Remove MuseScore menu item from musescore_menu
I assume this is something that wasn't cleaned up after your latest changes.
I would assume the menu callback for: admin/config/media/musescore is not needed anymore.

I would assume that all these issues are due to the fact that you haven't finished migrating your back-end configurations archicture entirely to musescore_block_configure (requested at #5 by jhaskins) and that this version is currently an intermediate between the one you initially presented and the final version suggested at #5.

All these issues could be fixed pretty quickly actually, since in any case this module is very short and simple. That would greatly help streamlining and improving the architecture of this module which would allow getting more adhesion from the community of contributors as well. If you were able to implement all the changes suggested above, I would tend to think, code would be cleaner, smaller, more consistent which would all contribute to greatly improving the reading speed and understanding another developper could get of it.

One last validation related advice would be for you to run the coding standards validation (PAReview) again, after you have committed your changes, to make sure you didn't introduce any new coding standards errors when you made the changes. After that, feel free to change the status back to needs review, that will save you one not so useful review from another user who would have simply told you about the coding errors mistakes (more back and forth = more time). This is something very common that happens to all of us (and just for one whitespace at the end of the line... the status is changed back to needs work again).

On top of all that, if you really wanted to do things the best way, I would recommend you copy paste some of the different items I listed/detailed above and make them tickets in your sandbox project tracker. Then, when you commit you will be able to include a ticket number in your log message, as it's being done with most standard contributed modules (Views, Panels, Search API, etc...).
This is not needed for the validation of the module, but think about its future and potential community of developpers that would greatly appreciate going through your documented work if they came to use this module. When the module is approved, the commits will be visible to users along with the issues and all that, which would really get your module to look like a real/serious one.

Obviously there is still more work needed on this module, whether critical issues or minor.
All of these items only engage my personal opinion and could certainly be discussed/argued, I just thought I could share with you how this module could be improved.

Well done and I sincerely hope these comments help you improving this very nice module.

Cheers!

Hi Dresden,

Thanks very much for your positive feedback.
I'm really glad to see you appreciated my report and that it helped steering you further into working in the Drupal community.

I tried looking into module's issue tracker but was unable to find tickets, so let me reply here directly.
Otherwise, if you post the tickets/related issues links for each one of the points, I would surely be happy to work in the tracker.
Especially since you still need some major changes, it would certainly be clearer. Once issues are solved/closed, the links would display as such on this page (See special syntax to link to issues in formatting tips), with a strikethrough (which I think makes it look pretty cool and would further demonstrate your developper's skills or ability to manage a project on a longer term).
Another great tip with that in my opinion, is that in module's tracker and issues, we would be also able to send you patches :-), that you would be able to roll in, keep us as authors/contributors, so that everyone keeps a record of it and you keep control on the development. I would surely be glad to contribute patches to your issues or features requests in the module, as I have done it in the past for others (For example: #1856218: Add a default configuration for Views meta tags).

Anyhow, here are my answers:

1 - Major problem with variable: musescore_num

1. I added the user configurable musescore_num variable back in, but this time I placed it in the "settings" form, per your suggestion. I had to limit the number to 10, as I don't wish to make too many requests at once to the musescore.com server when I validate the scores every time the block is saved. I think 10 scores should be enough for one block, in any case. (Perhaps I can add an option later to "clone" the block?) I also updated the Configuration instructions in the README.txt to reflect this change.

Alright, sounds good, then what I would do would be the following:

1. Add a constant variable to the module, for example MUSESCORE_NUM_MAX:
   

   /**
    * TODO: Remove this line, this is an example to be edited.
    * Define the maximum number of requests to musescore.com server
    */
   define('MUSESCORE_NUM_MAX', 10);
   

2. Change the description text on the block configuration form for the musescore_num field, to dynamically display the limit, for example:
   The maximum number of requests to musescore.com server is [MUSESCORE_NUM_MAX]
3. Make dropdown options dynamic to loop on a range and please remove hard coded options, to be replaced with something like:
   

   drupal_map_assoc(range(0, MUSESCORE_NUM_MAX));
   

   if possible. FYI: drupal_map_assoc($array, $function = NULL)

4. Change the validation hook to prompt and error message with the max limit number displayed in the message, for example:
   Sorry, the maximum number of requests to musescore.com server is [MUSESCORE_NUM_MAX]

2 - Remove path key from $form array in musescore_block_configure

2. Removed ['path'] from the form array (a copy pasta oversight).

I have searched for the string "path" in musescore.module and couldn't find any match.
This item should be fixed.

3 - Merge all the variables references into a serialized array

3. I had some troubling Merging all the variables references into an array. Saving the variables was fine, but problems arose referencing them from within the for loop. I'll come back to this.

I haven't checked this part in your code yet, but let's keep this item open and come back to it after you've done more tests and request a review on that.

4 - Simplify the CSS and make it dynamic

4. Simplified the CSS by adding the suffix/prefix div/classes.

Exactly what I had in mind:

• musescore.css: css file, looks good.
• musescore.module: module file, looks good.
  Added the prefix and suffixes properties as suggested.

This item should be fixed.

5 - Maybe try to stick with forms

5. Would I create a new function for musescore_block_configure_submit? I came across conflicting documentation on this function and became somewhat confused.

Now you have attracted my curiosity. I think both are possible, it's simply the module examples I found and I used around usually work with forms, like for example:

• The highly popular Internationalization (i18n) module does it in:i18n_block, see: i18n_block_form_block_admin_configure_submit
• The very popular Block Class module does it in:block_class.module, see: block_class_form_submit
• Lastly, the Core Block module, seems to do it as well in: block.admin.inc, line 466, see: block_admin_configure_submit

Given the number of users for these different codes, I would feel rather safe going along the same way.
But I would surely be glad to hear if you would have any other comments, recommendations or best practices guidelines, about that.

6 - Watchout for the variables to be deleted upon unistall

6. Fixed the variable_dels in the .install by adding a loop. Will need to change this again when I figure out the problem is was having with variable_get(array) above.

Since this item is closely related to point 3, I recommend we keep it open for now and that you monitor it again after the variables have been stabilized.

7 - Remove MuseScore menu item from musescore_menu

7. Removed the redundant musescore_menu() function.

Looks good, checked code in musescore.module, function musescore_menu() and the item was removed indeed.
This item should be fixed.




Let's sum things up, shall we?

1. Major problem with variable: musescore_num: Needs work.
2. Remove path key from $form array in musescore_block_configure: Fixed/Closed.
3. Merge all the variables references into a serialized array: Needs work/In progress.
4. Simplify the CSS and make it dynamic: Fixed/Closed.
5. Maybe try to stick with forms: Feedback.
6. Watchout for the variables to be deleted upon unistall: Feedback/Pending item 3.
7. Remove MuseScore menu item from musescore_menu: Fixed/Closed.

It seems you've already taken down most of the issues quickly, well done!
I hope these comments confirm your efforts are paying off and that very soon your module should be able to go through other coders hands.

Keep up the good work!
Cheers!

Dresden.... dude....

Don't go on and bug our dear klausi that's so busy reviewing applications, by posting features requests, bug reports and support requests in this issue queue :-).
#1862802: MuseScore Review #3 - Merge all the variables references into a serialized array
#1862804: MuseScore Review #3 - Simplify the CSS and make it dynamic
#1862790: MuseScore Review #3 - Major problem with variable: musescore_num
#1862812: MuseScore Review #3 - Watchout for the variables to be deleted upon unistall
#1862814: MuseScore Review #3 - Remove MuseScore menu item from musescore_menu
...

All these issues should be posted in this project's tracker: Issues for MuseScore.
Create new issues there: Create a new issue for MuseScore.
Or, I'm not sure if you can but please try to move the tickets you have already created to the correct project, by editing the field project of the tickets.

I'll subscribe to your issue queue now.
Good work, you've created many tickets.... they just need to be placed in the right queue now :-)

Cheers!

No worries at all Dresden. I found it funny actually :-)

In fact, I'm really glad to see you went ahead and created the tickets (So I gave you a quick help and changed the tickets to Musescore's tracker).
That will greatly help structuring this discussion and keeping track of changes for your module.

Now, please update all the tickets statuses and other fields, such as categories (feature requests, bug reports, etc...), component, etc.... you're the one in control now.

Then please post the issue numbers here, if possible, with the blockers that will make things easier for any of us to help you fix them.

Keep up the good work Dresden, Musescore's module page now has issue number >0, which is already a very good start towards adhesion of your module from the community.
Cheers!

Hi dresden,

I sincerely apologize for this late reply... I've been overly busy lately and taken by several cases that couldn't wait (been already delaying...).
Anyway, I have taken the time to go through your module again and test it on a clean install.
I have given several different parameters for the max number, the fields title or URL, etc.... and there has been a tremendous improvement.

Getting back to our issues:

#1862790: MuseScore Review #3 - Major problem with variable: musescore_num
I like the way you've changed the musescore_num to a dropdown in the block settings pages, with a default value of 3 along with a nice description text.
See: admin/structure/block/manage/musescore/musescore/configure
In the code you have defined a constant variable which makes it clear, clean, simple and easy for other developpers to change this limit if necessary.
All these changes related in code have been well implemented in other functions, validate and save.

#1862804: MuseScore Review #3 - Simplify the CSS and make it dynamic
What a great improvement! (From the first version I saw with 10 different field class IDs or classes....)
You've even added the margin-right: 10px; for class .musescore-title-field.
I've tested it and it looks really clean now, even with different numbers of fields, max and min included.

#1862802: MuseScore Review #3 - Merge all the variables references into a serialized array
You've merged everything into musescore_vars which seems fine, since each time you're getting/setting this variable, you need to manipulate all the variables:

• musescore_num
• musescore_title_%
• musescore_url_%

- Setting them on the block configuration form page.
- Getting them on display.

This change greatly simplified the .install file and in particular #1862812: MuseScore Review #3 - Watchout for the variables to be deleted upon unistall, which all seem fine now.
I have tested and checked in the variable table and everything seems fine.

#1862810: MuseScore Review #3 - Maybe try to stick with forms
For this issue, I have checked further your code and in the Examples for Developers module, in block_example.module and it seems indeed, that what is being used along with hook_block_configure() is hook_block_save().
(As it is in the module currently)

So I guess I got confused since if you check carefully in the examples mentioned above, in #10:

Now you have attracted my curiosity. I think both are possible, it's simply the module examples I found and I used around usually work with forms, like for example:

• The highly popular Internationalization (i18n) module does it in:i18n_block, see: i18n_block_form_block_admin_configure_submit
• The very popular Block Class module does it in:block_class.module, see: block_class_form_submit
• Lastly, the Core Block module, seems to do it as well in: block.admin.inc, line 466, see: block_admin_configure_submit

These modules don't seem to directly use block hooks, such as hook_block_configure() to alter the configuration form, they actually seem to be using form hooks hook_form_alter() or hook_form_FORM_ID_alter() and that's probably why they would also be calling _validate and _submit callbacks, rather than hook_block_save.
I think I got clearer on this as well.

I have tested your code and everything seems to be validated and saved correctly.
Please find attached with this comment a quick screenshot to demonstrate the results of my tests.
File named: muse_score-test-block-screenshot-1857646-16.jpg
I've got my scores listed in a block and a page where its embedded: looks pretty cool with the player and all.

Automated code validation (PAReview) seems clean as well.

One quick thing, perhaps: I would recommend you add a quick note to developers in the README.TXT file,:

Note:  At present, the Flash browser plug-in is required to play back
the scores.
For developers: If you wish to display more than 10 items, this could be 
changed in the modulefile, simply change the constant variable 
MUSESCORE_NUM_MAX to the value of your choice.

That's all I could find for now, but with a little bit more time, I might be able to find other items that could potentially be improved.
In any case, I would probably be posting in module's tracker.

Well done dresden! You've done a very nice rework of the module and come a very long way already.
I will keep following the ticket and see if I can give any more advice/comments.

At this point, I think you would probably need to get other peers reviews as well, with fresh eyes and testing hands to bring new review angles/validation aspects to this module's review.

Feel free to let me know if you would have any more questions, comments or issues on any aspects of these tickets or previously discussed items, I would certainly be glad to help.

I'm looking forward to seeing more work done on this great module and hope my comments were for the best.
Cheers!

✓ Automated Review

BOOM! No errors or warnings!

Nicely done!

✗ Manual Review

• ✓ README.txt -- Clear directions; followed them and was able to do as they say.
• ✓ musescore.css -- Minimalist use of CSS. No issues here.
• ✓ⓘ musescore.info -- Looks good, though my one quibble would be that the package probably shouldn't be "Media". Media is an existent, well-known module that has nothing to do with this one. I'd say you should either leave this line out (Putting it in the "Other" category on the Admin ➥ Modules page), or change it to "MuseScore". I don't think this should block RTBC, however.
• ✓ musescore.install -- Good that you added an install/uninstall routine, though the former seems unnecessary considering all you do with it is set a default for musescore_vars' musescore_num attribute (You can set defaults during musescore_block_view() using the second argument of variable_get() -- see: http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/varia...). Good that you remove your variables during uninstall, though.
• ✗ musescore.module -- Pretty much everything's in your .module file. In depth:
  • ✗ ln. 14 -- You should really end your block comments with the closing */ on its own line.
  • ⓘ ln. 25 -- If you wanted to avoid having to do an install routine, you'd just define the defaults for that variable here.
  • ✗ ln. 31 -- You're doing strings wrong. In order to get around the "no concatenating translatable strings" warning from PAReview, you're concatenating a bunch of translation strings.
    • What you have: '#description' => t('Enter the title and the MuseScore URL for the scores you wish to display. Visit ') . l(t('www.musescore.com'), 'http://www.musescore.com', array('html' => TRUE, 'attributes' => array('target' => '_blank'))) . t(' to search for scores.'),
    • What you should have: '#description' => t('Enter the title and the MuseScore URL for the scores you wish to display. Visit <a href="@url" target="_BLANK">musescore.com</a> to search for scores.', array('@url' => 'http://www.musescore.com')),

    See: http://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

  • ✗ lns. 43, 44, 61, 80 -- Should use t().
  • ✗ lns. 103, 107, 113 -- Should use t(). ln. 107 has same issue as ln. 31.
  • ⓘ ln. 143, 154 -- I don't think you need to translate "MuseScore" as it's a trademark and probably won't change across languages.
  • ⓘ ln. 156 -- If wanting to do what I mention at ln. 25, you'd also have to do it here.
  • ✗ lns. 162, 177, 179, 189, 191 -- You're using path arguments incorrectly. You should supply the URL-encoded URL as an argument to hook_menu, ensure it's sanitized in some fashion (It goes through a FAPI text field, so you've already done this) and then supply it as an argument when defining the musescore_scorepage() function. In general, try not to use $_GET for anything if at all possible. Adding "PAReview: Security" tag because, while MuseScore's API currently doesn't return anything if given an invalid MuseScore URL, there's always the possibility that gets changed and the string supplied gets returned in some fashion, potentially allowing a XSS vulnerability.
    
    In short, use hook_menu arguments and ensure that whatever argument you receive a path gets sanitized at some point (Probably within muse_scorepage()). See: http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...
  • ✗ ln. 162 -- As mentioned above, you should probably urlencode() the URL you're supplying as an argument. My MAMP install handled it fine, but I can see something like Nginx or a badly configured Apache server choking on it.

You're definitely getting there! Just a few small fiddly things.

ⓘ Suggestions

1. Do the title/URL fields as AJAX repeating form fields. It's much better for UX if you have a single field you can click "Add another" to spawn more instances of, instead of defining how many to create somewhere and having a hard limit of 10. Probably not necessary for the initial release, but definitely something you should consider for latter versions.
2. It'd be really cool if there was some way of having an autocomplete search for MuseScores on the block config page. Look at http://drupal.org/project/examples to get an example of how to code one of these -- they're really pretty easy! Again, probably for a latter version.

Keep up the good work! :)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

Updated sandbox git clone command -- was using author's user ID before.