Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
14 Jan 2014 at 22:40 UTC
Updated:
11 Feb 2015 at 22:14 UTC
Jump to comment: Most recent
Comments
Comment #1
sammuell commentedComment #2
PA robot commentedWe 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.
Comment #3
devd commentedPlease resolved the following error.
FILE: /var/www/drupal-7-pareview/pareview_temp/cleanup_translations.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
32 | WARNING | Hook implementations should not duplicate @param documentation
Comment #4
klausithat minor problem is no application blocker on its own, any other issues that you found or is this RTBC?
Comment #5
tomasribes commentedHi, I have been testing your module in my development site. There is no much obsolet translations strings there and the cleanup spends a lot of time (about 3 minutes for only 1 string to be removed).
Looking at your code, it seems good work! And the long execution time comes from potx file analysis and cleanup_translations_batch_process() anidated iterations.
Comment #6
ajitsAutomated Review:
There is a single issue that is reported by pareview. Please clean it up.
Manual Review:
Both in
function cleanup_translations_form()andfunction cleanup_translations_form_submit().It will be called everytime the form is rendered and also when it is submitted.
From the comments
I see that this is used to remove all the data from unfinished batch. This should not be handled here. It is already being handled in the
function cleanup_translations_batch_finished()where you callreset($operations);which resets all the operations if the batch remains unfinished.There is no need to assign this issue to yourself. It could be misleading for other reviewers.
Comment #7
sammuell commentedThanks for the reviews!
I've added a few things:
Because the module parses every file in your Drupal installations it might take some time. It does not matter because usually you're not doing this every other hour.
The issue reported by pareview was actually misleading, as the reported function is not a hook function.
Why is that so?
Comment #8
sammuell commentedchanging the status...
Comment #9
izus commentedHi,
i read the code and here are my notices :
i think that it is not a good idea to overrite a Global variable like $_potx_strings, i suggest working on a copy of it and leaves the Global $_potx_strings defined by potx module lives its live :)
locking for 10 minutes may not always be needed. i suggest the lock time should be configurable throw UI with 0 as no lock value and/or that a link suggests to breack the lock if the user knows what they do and have the permission to do so.
Thanks
Comment #10
sammuell commentedThanks for the review, izus. I disagree with you on the two mentioned points, so i'm resetting the status.
I'm also not comfortable with the situation, but that's the only way to do it. I don't know why potx used a global variable instad of the drupal_static() pattern. That way it would be nice and clean to get or reset the strings.
I think that's unneccessary. Checking for outdated string entries is not a task that is run every day. The most likely use case for this module is on a developer or tranlation installation, which typically are not accessed by many people at the same time. Also, a user who knows what to do would not disable or break the lock ;-)
Comment #11
Noe_ commentedLGTM
Comment #12
pingwin4egHi @sammuell
I like the idea of your project. I haven't tested it yet, and I have some questions about it. Does it take care of strings which aren't in code, for example fields labels, strings from Views, etc? And is your module compatible with i18n (in particular with i18n_strings) and other popular modules that work with translations? If so, this information can be added to the module's description.
Also some recommendations for you:
As can be seen from your project page there's no your name in 'committers' block. Please read the Identifying yourself to Git and included docs.
Also you can help with reviewing other project applications to get a review bonus. This will put your project on the high priority list, then git administrators will take a look at your project almost right away.
Comment #13
kscheirerChecked for security, licensing, Drupal API, and individual account issues, none found. There are some minor style issues reported at http://pareview.sh/pareview/httpgitdrupalorgsandboxsammuell2166669git.
Thanks for your contribution, sammuell!
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:
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.