This module makes it possible to select and collect content from
The Guardian newspaper for reuse on your Drupal site via the Open Platform API.

You can use this module to get access to over 1 million articles dating back 10 years in addition to the most recent content published on a daily basis.

Straight after installing this module you will be able to display a block on your page showing the latest headlines based on your search criteria. You will also have the option to display thumbnails in the block along with short trail text if you so wish.
Set your search criteria at admin/config/services/open-platform

In order to publish an entire article on your site you will need to get an API key which can be easily obtained from here http://guardian.mashery.com/.
Once you have an API key go to admin/config/services/open-platform and add your API key to the field labelled API KEY.
You can now go to admin/config/services/open-platform and publish content returned from your search query in your site.

This module will appeal to any kind of site that would like to offer its users the most up to date news. Or maybe you would like to show the latest news articles based on a particular topic on your site and allow your users to have open discussions on the content.

Project page

https://drupal.org/sandbox/t14/1974838

Git repo

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/t14/1974838.git

Reviewed Projects

https://drupal.org/node/2066099#comment-7771233
https://drupal.org/node/2064317#comment-7769955
https://drupal.org/node/2088391#comment-7858599

https://drupal.org/node/2104683#comment-7934279
https://drupal.org/node/2097475#comment-7948275
https://drupal.org/node/2094477#comment-7948345

Comments

t14’s picture

Issue summary: View changes

Added links to reviewed projects comments

PA robot’s picture

Status: Needs review » Needs work

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

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.

t14’s picture

Status: Needs work » Needs review

Fixed.

bappa.sarkar’s picture

You can use drupal_http_request($url) rather open_platform_get_content($url)

abghosh82’s picture

Status: Needs review » Needs work

I like the module and idea for getting news from open platform is good, following are some of minor code review suggestions:

open_platform.module - On line no. 215, 252 please use some short descritive variable name for $x.
open_platform.module - On line no. 259, instead of using 'img' tag you can consider using theme_image.
open_platform.module - Functions 'open_platform_get_dummy_data' and 'open_platform_formatted_data' etc. you can consider these function which I belive is to povide mock data for testing into a sreperate inc file wich can be included in the test. This is just to make the main functional code separate from solely test related code

Other than these I installed the module, some notices and warning that I got:

On 'admin/config/services/open-platform'
Warning: Invalid argument supplied for foreach() in open_platform_get_end_points() (line 309 of /var/www/html/d7/sites/all/modules/custom/open_platform/open_platform.module).
Notice: Undefined variable: endpoint_values in open_platform_get_end_points() (line 329 of /var/www/html/d7/sites/all/modules/custom/open_platform/open_platform.module).

On 'admin/structure/open-platform' got this for the first time, later it went off.
Recoverable fatal error: Argument 4 passed to open_platform_options() must be of the type array, null given, called in /var/www/html/d7/sites/all/modules/custom/open_platform/forms/open_platform_preview_form.inc on line 23 and defined in open_platform_options() (line 127 of /var/www/html/d7/sites/all/modules/custom/open_platform/open_platform.module).

Also noticed on you issue description the git clone link for non maintainer is not the correct one, I think use the one on your project page.

abghosh82’s picture

Issue summary: View changes

link to review comment

wesleydv’s picture

open_platform.info
3: I don't this is correct

open_platform.module
97: This requires that CURL is available on server: mention on project page or consider replacing by drupal_http_request().
114: Typo
120: Typo
133: Remove empty line
142: Remove empty line
157: Typo
158: Try keeping consistency in use of quotes vs double quotes
172: Typo
197: Typo
284: Remove empty line
304: Indentation looks a bit off
317: Indentation looks a bit off
469: Typo
542: Typo
576: Unused parameter $index
631: I think mocking code should be in the actual test not in the module file (not sure)

open_platform_block.tpl.php
8: Remove line
9: Remove empty line
35: Remove extra spaces

open_platform_admin_form.tpl.php
5: Typo

open_platform_preview_form.tpl.php
5: Typo
49: Typo
130: Typo

open_platform.css.css
Remove double file extension

t14’s picture

@bappa.sarkar Agreeded drupal_http_request would better suited for this use case. It also means user do not have to worry about having curl setup.
Changes made and commited

t14’s picture

@wesleydv Thank you for your review. Made the changes you suggested.
With the exception of the "576: Unused parameter $index" this is needed for the array recursive function in the .module file on line 216.

@abghosh82 Thank you for your comments, I applied your suggestions will fix the php warnings and put this back to "needs review"

t14’s picture

Issue tags: +PAreview: review bonus

adding a review bonus tg

t14’s picture

Status: Needs work » Needs review
rameshrasaiyan’s picture

Status: Needs review » Needs work

Hi,

I manually reviewed your code and found few minor things.

In the open_platform_preview_form.inc file you have used a img tag ('thumbnail' => "<img src='" . $thumbnail . "' />"), instead you can use the theme_image() Drupal API.

And in the open_platform.module file few items are missing the t().
For example:-

'title' => 'Guardian Open Platform settings',
'description' => 'Configure Guardian Open Platform settings',

Also I don't think you required this "open_platform.test" file in the repo.

Thanks,
Ramesh

t14’s picture

Status: Needs work » Needs review

Thanks for your review.

Made the changes you suggested. However the open_platform.test is need for doing simple test. There is a nice tutorial in the link below if you will like to know more about simple test.
https://drupal.org/simpletest-tutorial-drupal7

Thanks
T

jojototh’s picture

Status: Needs review » Needs work

Hello,
you have some style issues in your code, you can check it here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxt141974838git

Also I run your module through coder module and I get these errors:

open_platform.module

severity: normalreview: i18n_10Line 20: Menu item titles and descriptions should NOT be enclosed within t(). [i18n_10]
    'title' => t('Guardian Open Platform settings'),
severity: normalreview: i18n_10Line 21: Menu item titles and descriptions should NOT be enclosed within t(). [i18n_10]
    'description' => t('Configure Guardian Open Platform settings'),
severity: normalreview: i18n_10Line 31: Menu item titles and descriptions should NOT be enclosed within t(). [i18n_10]
    'title' => t('Guardian Open Platform content'),
severity: normalreview: style_uppercase_constantsLine 258: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE [style_uppercase_constants]
      'attributes' => null,
severity: normalreview: style_else_spacingLine 315: else statements should begin on a new line [style_else_spacing]
      }else{
severity: normalreview: style_else_spacingLine 332: else statements should begin on a new line [style_else_spacing]
      }else{

Actually titles in menus should not be enclosed with t() function, because it would cause that the string is transformed 2 times through the function. The title is automatically passed to title callback, which is by default set to t() function.

https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

"title callback": Function to generate the title; defaults to t(). If you require only the raw string to be output, set this to FALSE.
open_platform_preview_form.inc

severity: normalreview: style_uppercase_constantsLine 59: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE [style_uppercase_constants]
      'attributes' => null,
severity: normalreview: i18n_11Line 128: The $string argument to t() should not begin or end with a space. (Drupal Docs) [i18n_11]
    drupal_set_message(t('You need an API key to publish content on your site you can obtain an API key here ') .

The last thing which I noticed is your implementation of hook_block_view. Move all code in open_platform_block_view into your case. (case 'data':). For now all the code which is not in this case is called for each block.

Otherwise it looks fine.

t14’s picture

Status: Needs work » Needs review

Thank you for your review.

Fixed the issues you pointed out

Thanks
T

t14’s picture

Thank you for your review.

Fixed the issues you pointed out

Thanks
T

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

Review of the 7.x-1.x branch:

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
    FILE: /home/klausi/pareview_temp/open_platform.module
    --------------------------------------------------------------------------------
    FOUND 3 ERROR(S) AND 3 WARNING(S) AFFECTING 5 LINE(S)
    --------------------------------------------------------------------------------
     220 | WARNING | Only string literals should be passed to t() where possible
     222 | WARNING | Line exceeds 80 characters; contains 81 characters
     222 | ERROR   | Whitespace found at end of line
     313 | WARNING | Only string literals should be passed to t() where possible
     535 | ERROR   | Return comment must be on the next line
     588 | ERROR   | Return comment must be on the next line
    --------------------------------------------------------------------------------
    
  • DrupalPractice has found some issues with your code, but could be false positives.
    
    FILE: /home/klausi/pareview_temp/forms/open_platform_preview_form.inc
    --------------------------------------------------------------------------------
    FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
    --------------------------------------------------------------------------------
     62 | WARNING | Do not call theme functions directly, use theme('image', ...)
        |         | instead
    --------------------------------------------------------------------------------
    
    
    FILE: /home/klausi/pareview_temp/open_platform.module
    --------------------------------------------------------------------------------
    FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
    --------------------------------------------------------------------------------
     260 | WARNING | Do not call theme functions directly, use theme('image', ...)
         |         | instead
    --------------------------------------------------------------------------------
    

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. open_platform.info: the "configure" link is wrong, does not point to your settings page?
  2. _open_platform_installed_instances(): the get_t() call is useless? Also elsewhere.
  3. open_platform_menu(): why do you use the "administer site configuration" permission here, does not really fit? You could create your own permission?
  4. open_platform_format_data(): this is vulnerable to XSS exploits. The data you are displaying comes form an untrusted third party source and therefore must be considered user provided input. You need to sanitize all user provided input before printing, make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

t14’s picture

Status: Needs work » Needs review
Issue tags: -PAreview: security +PAreview: review bonus

Hi

Fixed the automated review issues and also the manual ones.

open_platform_format_data() filters for XSS by calling array_walk_recursive which uses open_platform_filter_xss_array() as its callback function.

Thanks for your time
T

t14’s picture

Issue summary: View changes

non maintainer git clone

klausi’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Oh, sorry about the XSS false alarm then, seems like you already took care of that.

manual review:

  1. "$empty_text = 'This data is unavailable';": all user facing text must run through t() for translation.
  2. "variable_get('open_platform_api_key')": all variables defined by your module need to be removed in hook_uninstall().
  3. "watchdog('inside loop', $item);": looks like a left over debug statement that should be removed.
  4. open_platform_cron(): cron runs every 5 minutes on my production sites, so the nodes will be updated every 5 minutes since this always triggers a node_save(). Is that really needed? I think you should only save nodes if something actually changes. And queuing all items every 5 minutes seems a bit too much?
  5. open_platform_get_end_points(): do not line break translatable messages in middle in t(). Code lines can be longer than 80 characters, only comments should be in that limit.
  6. open_platform.info: dependencies are missing like text module and possibly others?

But that are not critical application blockers, otherwise I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to kscheirer as he might have time to take a final look at this.

klausi’s picture

Assigned: Unassigned » kscheirer

Trying to assign again.

kscheirer’s picture

Assigned: kscheirer » Unassigned
Status: Reviewed & tested by the community » Fixed
  • In open_platform_requirements(), you're not actually checking for curl, is that an oversight?
  • I'm not sure how block caching is handled in drupal core, but I'm not sure that adding your own cache layer is necessary. Could you just set 'cache' => DRUPAL_CACHE_GLOBAL in open_platform_block_info()? Also your cache_set is not cached for 30 minutes, it's actually permanent unless you provide a timestamp as the 4th parameter.
  • In open_platform_get_data() you can use http_build_query() to create the querystring right from an array.
  • In open_platform_format_data() just initialize $template_vars = array(); at the top, then you don't need that giant if statement.
  • In open_platform_settings_form() the $field_options should be run through t() as well.
  • In open_platform_preview_form.inc, I don't think you've actually implemented hook_form. That only gets called for node types you've defined in your module in a hook_node_info().

I'm not sure I agree with the general approach of the module. You've pre-defined quite a bit to make it useful out of the box, but also limited what can be done. Perhaps this is by design, but a much more Drupally approach would be to use Feeds (see https://drupal.org/node/856644) or let people have more control about the content type used for articles.

Most of the above are style suggestions, I don't see any blocking issues. I just read klausi's note above, and getting the dependency list into the .info file is pretty important.

Thanks for your contribution, t14!

I updated your account to let you 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 get 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.

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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

reviewed another 3 modules