[D7] Cel

Fix the PAReview issues in attachement first.
You can check here is your project meets Drupal coding standars http://ventral.org/pareview/httpgitdrupalorgsandboxpeter1509964git

It appears that there have been multiple project applications opened under your username:

Project 1: Cel
Project 2: Unleash the Drupal watchdog in jQuery

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

What an awesome sounding module! I think this would solve several problems I have.

Patrick: Do you have a crystal ball that tells you which one would be accepted first? Let's leave this open so it might have a chance to be the one that gets Peterx accepted.

I'm sorry, but there were several discussions about this on groups.drupal.org.
Multiple applications just double the workload we have, and we have a lot of it.
He can feel free to reopen this one but close the other one.
I closed this one because it was newer and probably take longer then the last one which is already on half way.

Yes, the problem is that these instructions are somewhere hardcoded in the project module, there are and where discussions about how to finally handle the master branch but this is still not resolved. pareview only suggests you to leave a readme rather then a single info file because many people were confused were the actual code is.

Also the 80-character per line issue should rather be understood as suggestion, if the code is going to look ugly because your following coding standarts that's really not the sense. We wont force applicants to follow all coding standarts perfectly, just whenever possible. (You can point people to this comment if they keep annoying you about it ;) )

I added some additional security checks to the pareview service client yesterday, so the captcha is (hopefully) not needed anylonger. I turned it off because many people were annoyed about it, so lets see if it is actually necessary.

The other issues are from drupalcs, please go ahead and create an issue about these points in its issue queue. Maybe your right and there's something to change, maybe you and me just don't know the reasons for these.

As said, take coding style as suggestion rather then policies, but whenever you can, follow them to help making code on drupal.org consistent.

thanks

Hi,

Manual Review :

1. Please use t() and placeholders for the description in cel.module line 75.
2. It seems you calling the cel_display_name() inbetween strings.Better you return the value to variable and use placeholders to use them.
3. Please avoid using HTML inside t() in line 129 of cel.module.
4. In cel_log() you are using Placeholders without t().It can be better if you enclose it.
ex:cel_log(t("abc"));
5. Use l() to create link markup instead of creating by your own in cel.install line 32.
6.It seems PAREVIEW contains some errors.so please get it cleared.

Thanks,

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.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• 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. You have to get a review bonus to get a review from me.

manual review:

1. cel_module_name(): that should be a constant, not a function.
2. cel_module_name(): link in the doc block is dead. Please add all your function documentation inline in code, you can generate HTML for your pages with doxygen, and possibly with the API module.
3. cel_block_view(): What about node access modules here? You should check if the user has access to that node before displaying it so that node access grant modules stay intact. See http://api.drupal.org/api/drupal/modules!node!node.module/group/node_acc...
4. cel_block_view(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as Cel content then I get a nasty javascript popup when the block is displayed. Anyone with the "Cel: Create new content" permission (or similar) can inject potentially malicious script stuff. See http://drupal.org/node/28984
5. /admin/config/system/cel: Fatal error: Call to undefined function cel_list() in cel.admin.list.php on line 33

You need to set the status to "needs review" if you want to get a review.

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you will not show up on my high priority list.

And please add all your reviews to the issue summary.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

manual review:

1. cel.class.php: coder_sniffer complains because you have a method cel() which has the same name as the class. Better rename it to avoid any confusion.
2. cel_token_info(): "t($name),": do not pass variables to t() where possible, because that cannot be found by automtic translation extraction tools.
3. cel.class.php: Please add all your function documentation inline in code, you can generate HTML for your pages with doxygen, and possibly with the API module.
4. Fatal error: Call to undefined function cel_list() in cel.admin.list.php on line 29

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

1. The Drupal way of documenting things is inline with doxygen in the code. If you decide to leave the project and shut down your documentation site then the docs are basically gone. Therefore the best practice is to have a lot of documentation with the code and additional documentation pages on drupal.org.
2. cel_admin_list_cels(): all user facing text must run through t() for translation, for example the words in the table header.
3. cel_admin_list_cels(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as Cel node title a get a nasty javascript popup at admin/config/system/cel_admin. You need to sanitize user provided text before printing, please read http://drupal.org/node/28984 again.
4. cel.admin.list.php: why is this file still needed?

The security issue is of course a blocker. The tag is used for statistics and to show examples of security problems.

Automated Review:
You say that there is only an issue here about a PHP4 constructor. The minimum PHP version of Drupal is 5.2.5 (http://drupal.org/requirements) so why do you have PHP4 style coding here?

Manual Review:

• Why do you have a function that returns the module's name? It seems very obvious that this always would be 'cel'.
• The names of your class files in the .info file doesn't have to contain quotes.
• In the overview page of content_types, the url to the documentation is rendered as plain text.
• What's the purpose of this module? I've created a new cel content item and nothing seems to happen there. Maybe you can give a better explanation in the module itself, instead of just pointing to an url.
• Why do you create a new content_type? Isn't it better to make use of the entity api?

Automated review

FILE: /var/www/drupal-7-pareview/pareview_temp/cel.class.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
45 | ERROR | PHP4 style constructors are not allowed; use "__construct()"
| | instead
--------------------------------------------------------------------------------

• The problem here is surely PAReview-related, but as these kind of things usually appear given exceptional circumstances, I looked into your code and found that your class name is Cel whilst you also have a protected property called cel. This overlap is the cause of this behaviour. Whilst it's not entirely incorrect, it's still bad practice. Code should always be readable and without even digging into the code, question is what is what? The class is Cel and the property is also cel? Please consider a better naming convention.

Manual review

1. Inline Doxygen documentation is missing for all classes (properties, methods). See http://drupal.org/node/1354#classes for the standard. Links to external documentation could still remain as an addition.
2. Log messages should not be translated (e.g. cel_log() ln 28) and should not be open to mutations over the translations.
3. No proper documentation in cel.module, e.g. @param, @return missing for cel_tokens_cel()
4. Instead of // Why custom? Document. use standard TODO, FIXME or XXX comments, e.g. // TODO: document or @todo … in function doc.
5. Remove useless comments, e.g. cel.install ln 17-19. It's much more effective to push some parts into a ticket, discussion, blog post.

Recommendations

1. You are using a getter and setter in one go, e.g. in Cel::cel(). Maybe it's not a problem in the circumstances, but how do I set a property to null? It's a good practice to use getSomething() and setSomething(), robustly.
2. Write self-explanatory code. I'd always prefer getLastItemOfDataCollection() to last() which often says absolutely nothing.

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.

Update of reviews list.