[D7] Skyword Integration

Hi, and Welcome!
I did a ventral.org test and here are the results: http://ventral.org/pareview/httpgitdrupalorgsandboxjmihalik2051227git

There are many warnings and errors reported by this report. Probably you need to configure your IDE to use correct line endings and indentations?

Manual review

- It looks like you have 2 semi colons in the .info file. If autoload module is a dependency, please uncomment that line. Drupal 7 requires php 5.2 already, so not necessary to explicitly require it here.
- You are using the variable system (system_settings_form()), so please add some variable_del() calls in your .install file to remove them during module uninstallation. If you don't need to add any variables during the installation, you can remove the hook_install implementation.

skyword.module

require 'skyword.conf.inc';
require 'skyword.admin.inc';

You are trying to include these files relatively to the Drupal root folder. As this is a 'require' call, this will trigger a fatal error.
Please do not include any file in your .module file's global scope unless you really need to do so.
menu hook (hook_menu) supports a file key that you can specify a file to include before calling any page callback function.

There are some concerns with checking permissions before creating a node on behalf of a user, making use of the permission system, etc but I do not have skyword access, nor the module worked during the installation so all my comments above are from what I can see from a normal git clone (7.x-1.x branch)

I've made the requested changes, but I'm getting an error about my README.txt not existing from paraview. The file is there though, so I'm not sure if I'm doing something wrong.

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.

@skyword, I think the problem is all your code isn't in the root of the repository, it's in another directory called 'skyword'.

Try out this patch and recommit, I think that error will go away.

I believe the needs work status is the appropriate one in this case.

I've moved the files and that took care of the README.txt issue, so I think this is all set.

hi Skyword,

- clean up your .info file.
- remove .install file.
- clean up CS
- use t() function (for example at skyword_validate_secret $error_data['message']).

changed status

I was hoping I could get some more info on what cleanup CS meant. The only error I saw from paraview before was an invalid warning about a variable being used.
Also what did we need changed in the info file?

@skyword - I think @snig was referring to the commented out code at the bottom of the info file.
;dependencies[] = autoload
It's not always 100% possible to lose all the commented out code, but it's a good guideline to delete it before your public releases. If it's not meant to be commented out then just lose the semi-colon.

Keep in mind the pareview.sh might pickup some errors that you can't fix for whatever reason. As long as you've fixed the ones that you can, you should be good to go.

Thanks @asherry. I think this is set now.

Project page

Please take a moment to make your project page follow tips for a great project page.

• You still have a couple of style errors reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxjmihalik2051227git.
• The url at the top of you .module file, I think you meant http://www.skyword.com?
• In hook_xmlprc(), skyword.delete_post is titled "Retrieves a post".
• In skyword_author_callback() init should also be the user's email address, and roles should be an array of roles to give them, or empty array.
• In skyword_post_callback() you're creating nodes with the 'full_html' input filter. You should make sure the author account can use this format with filter_access(), or document somewhere that all article authors need to be given access to this input format.
• You're also not checking that the author has access to create a node of the requested type. Or even that the requested type exists! This is a security issue.
• I'm not sure what you're doing in the /* Allow updates. */ section, can you elaborate on that? Generally vids are not the same as nids, and you don't have to update $node->changed manually, that will happen on node_save anyway.
• At the end of that function, for the link, you can just use url()with the absolute => TRUE option.
• In skyword_get_authors_callback(), if you only want the name and uid, just grab those values instead of foreaching through the entire user object.
• In skyword_get_tid_for_term(), why are you setting $vid = 1, how do you know what vocabulary is at vid 1?
• Also in that function, since you're saving $terms, won't array_keys($term); sometimes be undefined?
• In skyword_preprocess_page(), if you're only interested in node pages, use hook_preprocess_node() or hook_node_view() instead. The same applies to skyword_page_alter().
• Never set error_reporting() manually - not only can this fail on some hosts, you should respect the admin's php settings.
• What is url('node/1', array('absolute' => TRUE) in skyword_page_alter()?
• All variables you've created must be removed in a hook_uninstall().

Looks like a nifty module! Settings to needs work for the number of outstanding issues.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

We have made the changes called out by @kscheirer a year ago, and would like to have the project application re-reviewed.

Is there any chance of getting an ETA on when this module will be re-evaluated?

I would really strongly recommend doing the review bonus. Without it, there are almost hundreds of other applications in 'needs review' status, but if you have the bonus you're only in line with a few.

It also helps out the drupal.org maintainers that actually promote these projects as they don't have to provide as much feedback to all the other projects.

First of all, let me apologize for not responding to your replies lately. I have had a lot of changes lately, and couldn't respond to any project applications.

A few suggestions. None of them are release blocker per se, but I think they can be important.

- Using fields to store meta data. You are using skyword_create_field() to create fields, which resets the cache during the run-time. You can always use a custom table for that, and hook into node_save to save the data only when present. I have worked on a similar functionality that creates fields on-demand, but immediately realized resetting the cache is a problem that I cannot ignore. Besides, it's only 3 fields that would fit perfectly in a single custom table. You can use node_load to load the data, and they will be available to preprocess/view hooks.

- If possible, move the xmlrpc functions to a separate file. You can keep the function stubs in the .module, and the stub includes the file when called. On a far extreme side, the xmlrpc callback can be a static method of a well-encapsulated class. I haven't seen any D7 modules using this approach, though.

- Excellent use of preprocess hooks, file API, hook_menu, t(), and xmlrpc.

I don't see any release blockers now. Hopefully an admin will review this and change your account to allow full status. Good luck!

Thanks for your comments and review Ayesh.

I did a manual review of the project. There are a few stylistic issues I can see

In skyword.admin.inc, all your form fields use the t function for title and description except for skyword_adminpage. Also the markup and description fields exceed the 80 character line limit. You also have several long lines in the module file.

You may also consider adding better documentation in your comments for each function to clarify the return type and what parameters are passed to the function.

Other than that, the code looks good.

Forgot to change the status.

Why this module has another (with the same name) inside src/modules? Sounds like a bug or bad practice.

root/src/modules/skyword/skyword.info

and

root/skyword.info

It looks like something used for continuous integration. Something like http://reload.github.io/phing-drupal-template perhaps? Probably not the best idea to make assumptions about continuous integration at the drupal.org project level.

I would clean that out from the repository and let the individual customize as needed for continuous integration.

Fixed the odd directory structure. Not sure what happened there.

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/skyword.module:

FOUND 34 ERRORS AND 3 WARNINGS AFFECTING 25 LINES

• Doc comment short description must start with a capital letter
• Missing parameter comment
• Return comment must be on the next line
• Format should be "* Implements hook_foo().", "*

Manual Review

You should show your git clone command to help reviewer get your project instanly:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jmihalik/2051227.git skyword

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.

Hello,

• I notice that you create some fields inside your module, It is good to uninstall them in your hook_uninstall file.
• The admin area should maybe be in the install file so it's directly linked from the modules list. See https://www.drupal.org/node/542202#configure
• Your hook_permissions implementation has a minor issue: Description should start with a capital letter and ending with a dot "." You can follow the code as implemented here.
• In your code you can follow the https://www.drupal.org/coding-standards#quotes for your quotes.
• In your README.txt file you can follow the instrunction and template link here

That look like minor improvements for the module, but not application blockers. Anything else that you found or should this be RTBC instead?

Sorry about this late post.
This module adds a few meta tags and other tags to the htnl head section, and values are taken from the nose in context. Hiwever, those values are not sanitized, and drupal does not sanitize them for you.

Wrapping the #markup and meta tags content with check_plain() should do it.

PA Review Error:

FILE: /var/www/drupal-7-pareview/pareview_temp/skyword.module
---------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
---------------------------------------------------------------------------
72 | ERROR | Expected "if (...) {\n"; found "if(...){\n"
286 | WARNING | Only string literals should be passed to t() where
| | possible
---------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
46 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Those minor coding standard errors are surely not application blockers, please do a real manual review.

I have fixed all issues brought up by pareview.sh. If there are no other application blockers, can we please RTBC? This has been a long 6 month process since I've started working on this.

Switched application author to shammack.

This is no release blocker, but one issue left in the automated review:

FILE: /var/www/drupal-7-pareview/pareview_temp/skyword.module
-----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------
556 | ERROR | [x] There should be no white space before a closing ")"
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------

Manual review:
1.
On skyword.admin.inc you first store your description/markup into a variable, then add the variable to your form item.
It's better to maintain if you place these inline.

2.
You do not use proper function documentation in the skyword.install file.

The uninstall function comment should start with:
Implements hook_install().
See: https://www.drupal.org/node/1354

3.
Inside the skywords_create_metafields function you also store the description into a variable and then use it in your code:

$description = 'The Skyword Tracking Tag is used to provide article performance data to the Skyword platform. This must be included on the page the article is being displayed.';
    skyword_create_field('skyword_tracking_tag', 'Skyword Tracking Tag', $description, $content['type']);

Perhaps rewrite it to this for readability:

    skyword_create_field(
      'skyword_tracking_tag',
      'Skyword Tracking Tag',
      'The Skyword Tracking Tag is used to provide article performance data to the Skyword platform. This must be included on the page the article is being displayed.',
      $content['type']
    );

Other then that looks good, and should be released to a full project.

Re #40: well the doc comment needs some work but saving text literals to variables is alright. It makes the function call easier to see and interpret (by humans) it well knowing function parameters.

I'm setting this back to "Needs review" because they are easy to fix and not critical issues to block the release.

If you find any other issues about securit you, performance, etc, feel free to change status again.

You need to move your configuration page under the configuration menu.

Your settings page accepts everything, and states that settings are saved. - Probably a bug.

You are not checking the inserted value. You can insert any value. and the system still says its accepted.
It would be good usability to check if the inserted value looks like the validation key you are expecting.

In the queue for 2 years, whew! It even looks like you've been actively making commits this whole time, too. Hopefully you didn't give up on the review process!

Automated Checks

PAReview is reporting a bunch of minor code style issues have crept into your commits you've made since this was last reviewed.

FILE: /var/www/drupal-7-pareview/pareview_temp/skyword.module
---------------------------------------------------------------------------
FOUND 11 ERRORS AND 3 WARNINGS AFFECTING 11 LINES
---------------------------------------------------------------------------
475 | WARNING | [x] A comma should follow the last multiline array item.
| | Found: 'integer'
674 | ERROR | [x] No space found before comment text; expected "// If
| | taxonomy is for a list just save values" but found
| | "//If taxonomy is for a list just save values"
674 | ERROR | [x] Inline comments must end in full-stops, exclamation
| | marks, or question marks
780 | ERROR | [x] Expected 1 space after IF keyword; 0 found
780 | ERROR | [x] Expected 1 space after closing parenthesis; found ""
792 | ERROR | [x] Doc comment short description must end with a full
| | stop
800 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 1
804 | ERROR | [x] Doc comment short description must end with a full
| | stop
812 | ERROR | [x] Expected 1 space after "="; 2 found
814 | ERROR | [x] Expected 1 space after FOREACH keyword; 0 found
817 | WARNING | [x] A comma should follow the last multiline array item.
| | Found: name
854 | WARNING | [ ] Line exceeds 80 characters; contains 82 characters
862 | ERROR | [x] Expected 1 space after IF keyword; 0 found
862 | ERROR | [x] Expected 1 space after closing parenthesis; found ""
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 13 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Considering these are all minor (mostly spacing) issues and that you've had this application here for 2 years, I'm not gonna say any of those are blocking issues. Please try harder to make PAReview or Codesniffer part of your normal workflow when this goes live, though. Thanks!

Manual Review - Blocker

There is only one item I feel should be fixed before this can be RTBC, but you're really close! If someone else feels otherwise, I'll set to it RTBC without it fixed and file an issue in your issue queue about it.

Admin form validation approach (blocker)

You are validating the settings form now as requested in #45 but you aren't really following the patterns that Drupal developers hoping to contribute to your code might expect.

Book's Admin Page (note that the validate is set directly in the form it builds)
Book's Admin Validate Callback (it's located in the same file as the form it is checking)

It is very rare that you'll need to use hook_form_alter() on your own forms... best to just do it when you're building the form in the first place!

Please move the validation callback into your skyward.admin.inc file, and set the callback directly in skyword_adminpage() instead of skyword_form_alter().

Manual Review - Non blockers

README.txt

...
INSTALLATION
------------
* Install as you would normally install a contributed Drupal module. See:
https://drupal.org/documentation/install/modules-themes/modules-7
for further information.

REQUIREMENTS
------------
* No special requirements

INSTALLATION
------------
* Install as you would normally install a contributed Drupal module. See:
https://drupal.org/documentation/install/modules-themes/modules-7
for further information.

...

Configuration page href mistake

The anchor on the configuration page that it supposed to go to http://www.skyward.com is incorrectly written. It needs "http://" in the href attribute.

Use of user_load() in rapid succession

I'm not 100% sure how much of user_load() ultimately uses cached information, but code like

    if (isset($struct['user_id']) && user_load($struct['user_id'])) {
      $account = user_load($struct['user_id']);
    }
    elseif (isset($struct['username']) && user_load_by_name($struct['username']) != FALSE) {
      // User found.
      $account = user_load_by_name($struct['username']);

strikes me as a little... awkward. I see why you're doing it, but I'd be inclined to re-write your conditionals in such a way so that you don't need to call an entity load function immediately after you already called it.

phpinfo() in xml-rmcp response

Even though phpinfo() by itself isn't harmful, it does usually provide information that would make attacks via other vulnerabilities easier to complete. Someone would need to know the API key in order to even get this information, but I also think it would be kind to allow a Drupal admin to disable this one via your configuration page and leave it disabled by default.

Consider changing permission 'Access Skyword Admin' to 'Administer Skyward'

Mostly a semantic issue. "access" is about viewing information but not making changes, while your configuration is about actually "administering" and changing configuration.

Automated Review

No issues found. See http://pareview.sh/pareview/httpgitdrupalorgsandboxjmihalik2051227git-7x-1x

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.
In the 'CONTENTS OF THIS FILE' section you refer to sections that don't exist. Also, you have a duplicate 'INSTALLATION' section

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (+) hook_permission() defines the permission 'Administer Skyword'. I.M.O. this sould be lowercase. Furthermore, in hook_menu() the wrong permission is checked: 'Access Skyword Admin'. This permission doesn't exist.
2. (+) in r412 you refer to the callback skyword_get_post_callback. That doesn't exist?
3. skyword_adminpage() is a form definition, so it should be skyword_adminpage($form, &$form_state)
4. Since you're only checking in skyword_form_alter() if the form_id == 'skyword_adminpage', it's better to use a form id specific alter, like skyword_form_skyword_adminpage_alter(). See the API
5. The 'message' in r588 is not translatable
6. Multiple functions are missing the correct doxygen:
   • skyword_version_callback
   • skyword_author_callback
   • skyword_post_callback
   • skyword_delete_post_callback
   • skyword_new_media_object_callback
   • skyword_get_authors_callback
   • skyword_get_vocabularies_callback
   • skyword_get_vocabulary_terms_callback
   • skyword_get_taxonomies_callback
   • skyword_get_php_info_callback

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.

This review uses the Project Application Review Template.

To clarify, I think #6 is referring to params that aren't being defined in comments. Maybe Nitebreed can clarify if I'm wrong about the intent of that feedback.

Thanks for being so responsive to feedback! Things aren't quite ready for a stable release but it's pretty close.

One more suggestion would be to add a "configure" line to your module's info file so that it can be easily accessed on admin/modules.

@stickywes, you're right!

Hi shammack,
I have suggestion about these below 2 points
1. In skywords.admin.inc file for form fields set default values for the function variable_get(); Lines: 35,44,53,62,71,80

Ex: variable_get(VARIABLE_NAME, DEFAULT_VALUE);

2. In module file line number 313 hook_form_alter() you altered your form to add validation instead of you can directly define skyword_adminpage_validate() in skyword.admin.inc file.

Glad to see you're still working at this despite sitting at RTBC for 2 whole months!

Still passing automated PAReview checks; it notes that you don't have tests implemented but that isn't new feedback.

I don't think the issues in #53 are enough to hold this up from RTBC. Please put this back into "Needs Work" if you disagree.

@shammack - this is RTBC! Congratulations.

Have you reviewed any other modules yet? If you have, please add them to the issue summary so you can get the review bonus and get this module approved and promoted to full status.

Well done, you have shown amazing patience and perseverance. Thank you!

@shammack - Information on the review bonus system can be found here: #1975228: Review bonus

Bumping to critical

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to a full project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues (here you get to ask fans of your project to help you!)

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.