[D7] Rublon

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxrublon2146525git

We 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.

Coder review result:

Coder found 1 projects, 29 files, 23 normal warnings, 0 warnings were flagged to be ignored

All of them are indent problems. Guess those should be fixed to conform the coding style standards.

Hi rublon,

I can see you are new to the Drupal scene and have a few topics I want you to look at. Seeing your code I have no doubt you can tackle these problems.

Coding standards:

You still have a lot of errors regarding the Drupal coding standards as commented by the bot in the first comment:
http://pareview.sh/pareview/httpgitdrupalorgsandboxrublon2146525git.
They are mostly indenting and whitespace issues. On D.0. we increment indenting with 2 spaces instead of tabs like you do. If you would like to learn more about the coding standards you can do so here:
https://drupal.org/coding-standards

Libraries API

I do realise that you're using an external library that is generating all these errors. If you would like to clean that up I suggest you try using the Libraries API. This way you can keep te library outside of your project and others can reuse it. But that would mean that you should put the Drupal hooks back into the module. If you want to know how to implement external libraries you can read up on it here:
https://drupal.org/documentation/modules/libraries

Fix git clone command:

You should also correct your git clone line in this topic's description. I'm unable to clone your project with it. It's just a ":" sign that needed to be replaced with a "/". It's best to wrap the text in a code block. Something in the lines of this should work:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rublon/2146525.git rublon

The t() function:

Looking at your code I see you are using OOP. That's very good but above my own paygrade. From the little bits that I can check like the form alters and stuff I can see you aren't yet familiar with the best practices of security measures and translations in Drupal. You should wrap all your form titles and descriptions text in the t() function for translation. If you use any user defined text you should use placeholders that automatically takes care of sanitizing the text. Read up on it over here:
https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

The Form API:

• Also in the hookLoginForm function I notice you wrapping something directly in the markup. You should actually make use of the Form API's #prefix and #suffix for that.
• One other thing that could make frontend developers cringe is the fact that you are writing inline CSS in your markup. We use the Form API's #attached property for this. That way you can gather all CSS in one single file and it can be optimized and aggregated.
• If you want to add classes to the form again there's the Form API. Your first pick should be the #attributes -> class property. For wrappers through prefix and suffix I do see many people residing to inline classes though.

Good use of function commenting.

One thing I've also noticed is that you have excellent function commenting practices. Like declaring the variables, the return value, function references. That's something not everyone does in Drupal 7. My guess is that you have a programming background in IT schooling. You just have some other ways of doing stuff, therefore not less than Drupals way. But if you would like to get your project promoted to a full project there's no way around Drupal coding standards.

I think that's about enough material for you to start with.
Welcome to the Drupal community. Hope you will stick around.
We need OOP'rs like you :)

While going through the module i could get figure out couple of issues :

1. In the css file you have used

.rublon-seal {
  clear: left;
  text-align: right;
}

but there is no such property such as
clear:left.
Rather it is
clear:both
or if you want to align it to the left it should be
float:left.

2. Also there is an error in your module with .php files..

Method name "RublonButton::__toString" is invalid; only PHP magic methods should be prefixed with a double underscore

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

Lucas, thank you for pointing this out. I have now added my personal details to the profile. Hope that everything's good now :-)

Can this module use the libraries API (www.dgo.to/libraries) rather than bundling the third party app with the module? It makes it nearly impossible with the sheer volume of the code to do a proper code review. Here's some things to chew on until someone has time to do a more full review.

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226
remotes/origin/7.x-1.0
remotes/origin/7.x-1.x-dev

.info file:
You should add a package. Its optional but helpful. (https://drupal.org/node/542202#package)
PHP version: https://drupal.org/node/542202#php
Configuration: https://drupal.org/node/542202#configure

.install file:
INSTALL.txt says the module requires cURL so you should add a hook_requirements.
Example: http://drupalcode.org/project/oauth.git/commitdiff/8e2d9d8

No uninstall function to remove any variable_sets.

.module file:
require_once dirname(__FILE__) . '/libs/RublonDrupal.php';
Use https://api.drupal.org/api/drupal/includes%21module.inc/function/module_.... Or better yet, use www.dgo.to/composer_manager and/or some other autoloader.

* Implements hook_form_FORM_ID_alter() for user_profile_form().
This line should simply be Implements hook_form_FORM_ID_alter().. Everything else should go as new lines of comment.
* @see drupal_prepare_form()
This isn't needed.

* @see hook_user_view_alter()
This isn't needed in line 30. In fact the entire hook_user_view_alter is unnecessary.

* @see hook_user_login()
Unnecessary comment because of the Implements foo comment.

  public static function getRublonProfileId($uid = NULL) {
   ...
    $cache = FALSE;
    $user = user_load($uid, $cache);

No need to pass FALSE to user_load as that is the default.
if (!empty($user) AND is_object($user) AND !empty($user->uid)) {
Only need to check for $user as the interface for user_load will return FALSE if something unexpected happens.

public function callback() {
    self::disableCaching();
    require_once dirname(__FILE__) . '/RublonImplemented/RublonCallback.php';

Use module_load_include.

  /**
   * Add error message.
   *
   * @param mixed $error
   *   The error parameter, must be type mixed
   * @param string $notifier_options
   *   The notifierOptions parameter, must be type string
   */
  public static function error($error, $notifier_options = array()) {

Comment does not match function or method signature (at line 151 of RublonDrupal.php)

public function callback() {
    self::disableCaching();
    require_once dirname(__FILE__) . '/RublonImplemented/RublonCallback.php';
    try {
      $this->callback = new RublonCallback();
      $this->callback->run();
    }
    catch (Exception $e) {
      self::error(RublonCallback::TEXT_MSG_ERROR, array(
        'method' => __METHOD__,
        'file' => __FILE__,
        'exception' => $e,
      ));
      $this->callback->goBack();
    }
    exit;
  }

Constant 'TEXT_MSG_ERROR' not found in class (at line 721 in RublonDrupal.php)

Unused local variable $destination (at line 600 in RublonDrupal.php)

Variable 'response' might have not been defined (at line 502 in RublonConsumerRegistrationTemplate.ph)

Variable 'response' might have not been defined (at line 496 in RublonConsumerRegistrationTemplate.php)

Unused local variable $result (at line 20 in RublonText.php)

Unused parameter $options (at line 295 in RublonIssueNotifier.php)

Hi @RublonMichal. I've reviewed your module, and have a few suggestions:

1) In terms of functionality, I'm afraid I'm dead in the water on the first step found in your help text:
Q: How do I use Rublon?
A: In order to use Rublon in your Drupal installation please go to
"Your Profile -> Edit" section and click "Protect your account".
On said page, logged in as admin, I see this:
Rublon will be available to you once your administrator protects his account.
Since your account is protected by a password only, it can be accessed from any device in the world. Rublon protects your account from sign ins from unknown devices, even if your password gets stolen. Learn more at www.rublon.com.
There are no further instructions or options available to me.

2) In terms of code review, your module passes all my phpcs drupal sniffs, but it sure would be nice to see more notes in your code, particularly in the .module file. You have taken a hardcore oop approach, and I find it pretty confusing to track things down without some pretty extensive notes / explanations relating to each object.

3) It would be nice if you could add a configuration link to your info file, i.e.,
configure = user/{UID}/edit - not quite sure how you'll plugin in the user id, but something like that would be helpful, if possible.

Thanks for your hard work.

PAReview.sh actually still comes back with a long list of errors. Most have to do with parameter comments and seem not very important. Do take a look at some of the lines that comment on whitespaces being located where they shouldn't be - this kind of stuff tends to throw off diffs since it's hard to see with the naked eye.

@EthanT: Ran into the same problem configuring a user account. But I disagree on the doc comment for the OOP patterns - here's why: The method calls are actually meticulously documented in the class implementations. Adding more comments in rublon.module would be redundant, IMHO.

Automated Review

Review of the 7.x-1.x branch (commit e3f0d95):

PAreview still comes back with a lengthy list of errors. Most look very minor, but some could throw off tools like diff and grep when looking for specific patterns (example: space after ? operator). http://pareview.sh/pareview/httpgitdrupalorgsandboxrublon2146525git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template. One minor comment: The documentation should list OpenSSL as an explicit dependency, the same way it lists cURL and the minimum PHP version. Most systems have OpenSSL installed, but not all.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

This took quite some time to review due to the amount of code, but I have not found any issues in the drupal module that I am concerned about. However, when testing the application, a few things stuck out (only the first is related to the implementation of this module), but should probably be fixed anyway:

1. (*) The button to protect an account is not at all intuitive. It took me a while of staring at it before I realized it was something to click rather than a logo. This is important because it will keep people from finishing the install (see comments above - apparently I was not the only one)
2. (*) The QR scanner that is part of the Android app is extremely finicky - I had to hold it very still for 5 to 10 seconds to get it to accept the code. I think this will severely limit acceptance.
3. (*) Some of the automated email communication was in Polish, the rest in English. My Polish is not sufficient to understand everything, but it did seem like important enough to be translated.

Hi Michal,

I do realize that the three issues I raised have little to do with the Drupal implementation. I think the first one is probably the most important one and has to do with the fact that the message talking about securing the master account first is getting people to think in the wrong direction, especially since it does show on the master account's screen as well as everybody else's. I think my real question is, is this really a rule you need to have enforced? If not, you could avoid quite a bit of confusion by simply eliminating all that language.

The second issue is definitely outside the scope of the Drupal implementation. I just wanted you to be aware of it. I did confirm that the third one is no longer happening in the confirmation mails.

So in summary, I think you'll need to make a decision on number 1 and bring the status back to review. I'm happy to support it being sent to RTBC status after that.

Klaus

Tested and confirmed. Switched the status to RTBC.

Neat stuff!

Hm, a quick question about your user account ... in #2151429-9: [D7] Rublon you said, you've entered your personal data but none of the accounts you've answered with includes any personal information and still looks like a group account of your company. Apart from this you are always speaking of "we" ... are you developing this module as a single person or as a group?

I just ran one more PAReview scan to make sure nothing else has popped up in the meantime and it came back clean. No other objections were raised in the last few weeks, so I will switch the status to "Fixed".

re #24: The project still has to wait for a git admin to review the project and provide the appropriate access to @RublonMichal. Moving this back to RTBC.

re #23: I hoping to review this soon.

Manual Review

Coding style & Drupal API usage

List of identified issues in no particular order:

1. rublon.issuenotifier.inc: Unresolved variable or type EXTERNAL_FRAGMENT at line 78 & 85
2. rublon.drupal.inc: Argument type does not match (at line 465)
3. rublon.consumer.registration.template.inc: Variable 'response' might have not been defined (at line 498 & 504)
4. rublon.drupal.inc: Unused local variable $destination (at line 690)
5. rublon.issuenotifier.inc: Unused parameter 'options'. The value of the parameter is not used anywhere. (at line 295)
6. Remove the .gitignore. If you need to ignore the .project folder, then create a global .gitignore. For instructions see https://help.github.com/articles/ignoring-files/
7. .module file & other places: Don't use module_load_include in a global context. See the phpdoc for why.
8. .module file: _rublon_callback() => Could this move to rublon.drupal.inc?
9. I'm seeing several instances of sprintf and html in constants, rather than using Drupal's theme system. Drupal's theme system is intended for that purpose.
10. (+) hookUpdateMessage seems to assume that all administrators shoulds be able to activate. This shouldn't necessarily be true, given Drupal's roll-based access model.
11. (*) The menu callback for _rublon_callback() isn't using any access parameters. It should, rather than incorporating that into the page callback.
12. (*) hookMenu should use the default type for 'admin/config/rublon'. You want it to show up in the menu, not hidden.
13. (+) I can't see any good reason to use AND instead of &&
14. (*) getRegistrationForm(): Why can't this use Drupal's FAPI?

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Overall, the code is well written and documented. It is a joy to read. In general, usage of more of Drupal's FAPI and theme layer are areas of improvement. I'm fairly sure there are some things I've missed in my review above, however I simply ran out of time and wanted to get some feedback back, rather than sit on it for longer until I can complete the review.

Overall awesome module and service!

Blocking Issues:

1. In canActivate() you're checking for the administrator role directly. Instead you should be checking for a permission - either 'administer modules' or a permission defined by your module. In Drupal we define permissions, and then allow the site owner to configure which roles have said permissions. This is also important because Drupal's User 1 will automatically have all permissions, but may not have the 'administrator' role even assigned. Your checks for the authenticated role are ok though, since that will always be granted to any logged-in user.

Non-blocking issues:

• In rublon.install you have a module_load_include() right at the top - it's better to tuck that inside the function that needs it, similar to how you've already got it in rublon_uninstall().
• In rublon_sdk_loaded() at least one else/if can be combined into elseif
• I'm not sure what rublon_page_alter() is up to, but it looks like it presents a toolbar? This would be nicer in a theme function, some people might not appreciate the injected markup.
• Drupal code standards use && instead of AND
• In setRublonProfileId() you might be able to make use of db_merge() to simplify the code
• In getProjectURL() I think $base_url might be easier than url().
• In privateGenerateRandomString() you can use drupal_random_bytes() instead of mt_rand().
• In rublon.text.inc using single quotes instead of double is slightly faster
• Does the module only require curl when reporting issues back to Rublon? That's just a question. Is it possible for an admin to disable this function?

If you can resolve that one blocking issue, I'd be happy to promote this to RTBC.

I second the findings of the reviewers in #26 and #28.

With those fixed it is RTBC from me as well.

The use of an /assets folder that just has /css/rublon.css in it is perhaps a little esoteric by Drupal standards, but not wrong.

Documentation on project page and README are very good and enticing to read: I want a Rublon!

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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