[D7] HubSpot Blog Integration

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.

Hello HaroldRKnieriem,

1. In generally (all of your files), you leave too much space that does not meet the standards Drupal

eg.

$items['admin/config/services/hubspot_blog_integration'] = array(
    'title'            => 'HubSpot blog integration settings',
    'description'      => 'Provides HubSpot blog content access for Drupal.',
    'page callback'    => 'drupal_get_form',
    'page arguments'   => array('hubspot_blog_integration_admin_settings_form'),
    'access arguments' => array('administer site configuration'),
    'type'             => MENU_NORMAL_ITEM,
    'file'             => 'hubspot_blog_integration.admin.inc',
  );

Change them into :

$items['admin/config/services/hubspot_blog_integration'] = array(
    'title' => 'HubSpot blog integration settings',
    'description' => 'Provides HubSpot blog content access for Drupal.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('hubspot_blog_integration_admin_settings_form'),
    'access arguments' => array('administer site configuration'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'hubspot_blog_integration.admin.inc',
  );

2. In hubspot_blog_integration.admin.inc

Line 10 : add $form parameter to your function hubspot_blog_integration_admin_settings_form($form, $form_state)
Line 15 : add t() function : t('No Blog Selected')
Line 126 - 127 : Breaking the line within a string, within the t() function is not so cool for translation!

3. In hubspot_blog_integration.install

Line 14 and 30 : variable $module_prefix seems to be not used (t() on 'hubspot_blog_integration')
Line 15 - 16 - 17 : The values set out will be the same for all module's users!? These values coming from where and why are same to
all users?

4. In hubspot_blog_integration.module

Line 83 to 95 : your permissions ("administer hubspot blog" and "use PHP for tracking visibilit") are unused. Delete this hook or use
your permissions.
Line 148 to 152 : It will be better if you use lowercase letters in your parameters
Line 164 : add t() function : t('No Blog Selected')
Line 208 : If you handle boolean values - use TRUE/FALSE instead of 0/1.
If you need integer - cast them before they are needed - (int) TRUE is still much more readable.
Line 215 : Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075
Line 449 : add t() function
Line 475 : use constant REQUEST_TIME instead of php time() function

Hi @HaroldRKnieriem,

I guess I will start my review by making a few suggestions about the way you work with git. The first thing that stood out when I looked at your repository viewer was the commit messages. You shouldn't use the same message for all your commits, even though you are fixing things that were suggested by the same person. Ideally you would like to end up with a nice list of meaningful messages that help you identify exactly what was fix in each one of the commits.

For more info about how you should be making commits to Drupal, check out this page: https://drupal.org/node/52287

In regards to the code itself, it looks pretty good but I am going to make a few comments:

1. In hubspot_blog_integration.admin.inc
- Line 12: If $module_prefix is supposed to work as a const I would move it out the function and define an actual const.
- Line 13: Is there a reason to translate ":" in ": Connected"?
- Lines 15, 16, 82: Having a string (--node--) sprinkled around the good is not a good practice. Define and use a const instead.
- Line 97: I would replace the hard-coded options array with a helper function that returns an array to make the code easier to maintain.

2. In hubspot_blog_integration.install
- Line 9: You should have "Implements hook_install()." first and in another line a description of what you are doing in the hook. Hook documentations should always start with "Implements hook_NAME().".

3. In hubspot_blog_integration.module
- Line 8: Do you really want the client ID hard-coded in there?
- Lines 62 and 72: I would replace the underscores with dashes in the paths
- Lines 67 and 76: I suggest you implement hook_perm and define a permission that lets users access these menu items instead of using "administer site configuration". It gives the users a more granular control over who can do what on the site.
- Line 93: Define a const for this URL to make the code easier to maintain.
- Lines 96, 126: Define a const for this error number, same reason as above.
- Line 151, 181: Replace "--node--" with the const you will define.
- Lines 346, 353: Always use "Implements hook_NAME()." and in another line describe what you are using the hook for.
- Line 448, 511: Another hard-coded URL that could be replaced with a const.

I think that is it. At least it is everything that I have managed to spot :-)

Cheers!

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.

Project page

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

• In your .info file, the name "HubSpot Blog Integration Module for Drupal" could be shorter. We already know it's a drupal module, how about just "HubSpot Blog Integration". That would match your filenames better as well.
• You have a couple unused variables reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxharoldrknieriem200022....
• You should remove all variables in hook_uninstall
  • hubspot_access_token
  • hubspot_blog_integration_access_token
  • hubspot_blog_integration_blog_count
  • hubspot_blog_integration_refresh_token
  • hubspot_blog_integration_blog_list
  • hubspot_blog_integration_available_blog_list
  • hubspot_portalid
  • hubspot_blog_integration_next_execution
  • hubspot_blog_integration_expires_in
  • hubspot_blog_integration_expires_at
  • hubspot_blog_integration_scope_returned
• All variables name should be in your namespace and start with hubspot_blog_integration_
• All your requests out to HubSpot have a lot of duplicated code - checking for the access token, handling a refresh case, etc. Refactor this into it's own function instead, then you can use it whenever you need.
• You can use http_build_query to build up the querystrings for you.
• In hubspot_blog_integration_blog_list_data(), you can simplify
  

    $blog_title = (trim($blog_array->blogTitle) != '') ? "{$blog_array->blogTitle}" : "{$blog_array->webUrl}";
  
    $blog_guid = $blog_array->guid;
    $blogs_array[$blog_guid] = $blog_title;
  

  to just

    $blogs_array[$blog_array->guid] = !empty(trim($blog_array->blogTitle)) ? $blog_array->blogTitle : $blog_array->webUrl;
  

• In hubspot_blog_integration_node_data() you're setting the new blog posts to uid 0, which is the anonymous user, are you sure that's a good idea?
• Also in that function, you're setting the input format to "full_html", which the anon user normally doesn't have access to.
• Why are you calling node_save($node); twice?
• Can you describe hubspot_blog_integration_form_cron_run_submit() a little more? I'm not sure where that function is called or what it's for.
• In hubspot_blog_integration_cron_queue_info() the worker callback function doesn't exist, can you remove this function?
• Instead of hubspot_blog_integration_node_type_insert(), I think you want to use hook_insert().
• In hubspot_blog_integration_query_alter(), why are you keeping your content type out of search results?
• In hubspot_blog_integration_oauth_connect(), I don't think you need a drupal_goto() at the end?

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

Can i have a test Portal Id - so i can continue my review?

Manual review:

Module is clean and simple, well commented, gets the job done, i personally like it, i give the GO!

No duplication

No

This sounds like a feature that should live in the existing HubSpot project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the HubSpot issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

PS: There is a broken link to HubSpot.com on your project page.

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.