Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
feature
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
13 Dec 2011 at 20:39 UTC
Updated:
12 Jan 2013 at 12:45 UTC
Jump to comment: Most recent file
Comments
Comment #1
8bitplateau commentedComment #2
patrickd commentedYou got some coding standart issues (See http://ventral.org/pareview/httpgitdrupalorgsandboxdigitisation1365530git, you can use this site to re-test your self)
at least the following issues should be fixed before you switch back to needs review:
Comment #3
8bitplateau commentedI've installed php code sniffer on my development stack and am using that in Komodo to check as I go now.
Some errors however are confusing e.g '.=' creates error asking for a space before the equals sign ?
Comment #4
8bitplateau commentedchanging priority due to time.
Comment #5
patrickd commentedPlease give more information about requiremets, installation and configuration on your project page
Remove "version" from the info file, it will be added by drupal.org packaging automatically.
You still got some formatting issues, see http://ventral.org/pareview/httpgitdrupalorgsandboxdigitisation1365530git.
The problem your mentioning about ".=" is your indenting:
$output .= '<div class="annotateplayer">';should be
$output .= '<div class="annotateplayer">';you have to use variable names prefixed with your module name, aa_* could cause collisions.
you don't have to drop the table at uninstall, creating and dropping tables is automatically handled by schema API
it's not best practice to set variables on install, use the default value parameters instead
You got some text strings that are not translatable because you didn't use t()
I'll continue as you fixed these,
regards
Comment #6
8bitplateau commentedok thanks ..
Questions:
Got a new code sniffer error in drupal_set_message().
I want to have a varaible printed (which should be checked/cleaned?) and have it translateable so I used:
drupal_set_message(t(check_plain('LAME found at ' . $lame)), 'error');but this thows an error, what is the best way to acheive this ?
I am using PHPCS in Komodo but the output is slightly differnet to the automated one.
The standard I am using is from the codesniffer module, are you using the same ?
It probably something to do with the way my IDE implements it, or mine might be out of date.
I tried removing the 'drop table' from uninstall hook and did a few tests but the table is not removed on module uninstall with out it ?
thanks.
Comment #7
patrickd commentedPlease read the api documentation about t() it's more than just a translation function!
drupal_set_message(t(check_plain('LAME found at ' . $lame)), 'error');=>
drupal_set_message(t('LAME found at @lame', array('@lame' => $lame)), 'error');Have a look at Drupal Code Sniffer
Did you really 'uninstall' your module or just disable it? you have to disable it first then click on the uninstall tab and uninstall it.
Comment #8
8bitplateau commentedFixed the formatting with drupal_set_message(); e.g
drupal_set_message(t('@p', array('@p' => $p)), 'error');in one implementation I had an embedded link but it was at the end of the string so I used;
'#description' => t('LAME link ') . l(t('sourceforge'), 'http://lame.cvs.sourceforge.net/'),instead of using the array within t(), hope this is acceptable. I can see its more than just translation now, but the way it handles links it a bit odd.
I added
drupal_uninstall_schema('audio_annotate');to handle dropping the tables properly, it was missing! that seems to sort it.
I've nearly got DCS standard set up properly in komodo, just some output formatting to do, then I'll offer the configuration guide to the Sniffer module, I see there is config for Eclipse and other IDE's there.
But I think there are no more formatting issues (fingers crossed).
Comment #9
8bitplateau commentedI've added an my komodo configuration to the drupalcs module page.
and changed priority due to time (sorry hate to nudge).
Comment #10
8bitplateau commentednearly 4 weeks since last feedback from reviewer - sorry to hassle
Comment #11
8bitplateau commentedHello, I need a review please :)
Comment #12
8bitplateau commentedhello everyone, I could do with a review please , it's been a quite few months now but I know everyone is busy.
In case you didn't know : this module creates a sound-cloud like cck field for Drupal, people are downloading it from my github and sandbox so it would be great to get some more feedback and reviews so I can make it rock solid for those that want it.
see it in action http://audio.8bitplateau.net/test
thanks
Comment #13
patrickd commentedI'm sorry for that long delay!
There are currently hundreds of applications in the queue :/
We do really need more hands in the application queue and highly recommend to get a review bonus so we will(/can) come back to your application sooner.
Comment #14
novalnet commentedReview of the 6.x-1.x branch:
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.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #15
8bitplateau commentedok will do, I fully appreciate the challenge the queue creates. Off to help others now ...
Comment #16
8bitplateau commentedHI Novalet,
Thanks for looking, it did pass pareview at one point over at ventral but now clearly it does not!
I think I must be using an out-of-date version of the standards for PHPCS, and maybe it skipped the CSS and JS, I'll have to look at the Komodo tool I made to launch it.
The large amount of CSS errors are from the jQuery UI theme-roller CSS files. Perhaps if I link to them hosted on Google OR module depend the jQuery UI Theme module and reference them then I don't have to ship them with this module ? does that sound like a good idea ?
It would be good to actually get some feed back and help as well as the necessary automated code aesthetics fixes.
Did you manage to get it installed ?
thanks
Comment #17
8bitplateau commentedFixed all coding standards errors as per digitisation_errors.txt
http://ventral.org/pareview/httpgitdrupalorgsandboxdigitisation1365530git
I was using DrupalCodingStandard PHPCS standard not Drupal PHPCS standard - doh!
Comment #18
ryandekker commented@digitisation, this is a cool module! I got it installed and working. I verified it passed automated code review steps; looking good.
Install Process
I will say the install process was a bit cumbersome. There's a lot of modules/libraries at play here, and your module requires different setup than is typical (version of jQueryUI, jPlayer).
The aa_theme replacement point is a bit confusing; first time through I copied the entire aa_theme directory into my theme. Also, if someone is using a core or contrib theme (which I think is pretty common), using your module will require them to "hack" that theme. I think it makes a lot of sense to do this step for users, and it's not that hard. You can override the default jplayer_single.tpl.php by using hook_theme_registry_alter, and you can override the the css and js files the same way that jquery_update does it. (Bit of a pain in Drupal 6, unfortunately.)
Regarding the rest of the requirements for your module, you should implement hook_requirements to help users fix their installation. Check for things like:
Other Nitpicks
audio_annotate_admin_settings_validate()andaudio_annotate_admin_settings_submit()say jplayer, rather than audio_annotate. This is especially confusing given that you're working closely with jplayer.Feature Request
I think it would be very helpful to have an option that would hide the comments, since they are listed above in the audio area already. Pretty minor, but I think it would help clean up those pages.
Comment #19
8bitplateau commentedHi,
Thanks for a really helpful review and taking time to look over both the module and the code.
I'm going to take on board as much of that as I can, lots to do but all worth doing, and I agree with the feature request too, it makes sense.
I didn't know about the hook_theme_registry_alter, that makes a lot of sense.
back in a while .......
Comment #20
meloff commentedHi,
AA on D6 works fine, thank you for the work done. :) But I need a new jplayer for D7.. Should we expect a version for Drupal 7?
p.s. google translate
Comment #21
8bitplateau commentedI am yes! just been a busy and need to get the D6 version through first.
There is a D7 version working but very buggy, I'll github it and let you have a link.
Comment #22
klausiClosing due to lack of activity. Feel free to reopen if you are still working on this application.