[D7] PublishThis Feed API

Moving this to the correct issue queue. Some of the useful links that you need to follow

1. Apply for permission to create full projects
2. Project application checklist

Please make sure you go through these links and other links from these pages and make changes to the project accordingly. After you do so, you could change the status of this issue to needs review.

You should change your git clone line. You have written the maintainer git clone line. To correct this you shold go to your project page, version control tab, and there deselect the administrator option. Then press show and your git clone line will change. This is the line you should post in the project application issue.

Also there is a lot of information missing in the application. Please follow the instructions in your module page.

Assigning myself for a review.

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.

Hi sandeepscs,
1. Please remove any one README file from your application. There are duplicates.
2. You may remove DEVELOPER.txt and include those two lines in your README itself under a Note.
3. Please correct the Pareview errors of your application.
4. Please keep all the css files at one place. Move the form_theme.css to css folder.
5. In your .module file, under hook_block_configure(), you have called drupal_add_js() without any parameters. Please give the file name there which you want to invoke. Similar is the case in publishthis.api under get_bundle_auto_contents().
6. Instead of calling drupal_get_path() so many times, you can call it once and save it in a constance, and use it each time whenever required as you have done for other cases.
define('PUBLISHTHIS_MODULE', drupal_get_path('module', 'publishthis'));

Could not get the opportunity to go through the project in details. Hope to get the time soon.

@AjitS, please check for the security requirements.

I have resolved the problems reported by ajalan065

Hi mangeshrs,
This application was submitted by sandeepscs.
So how come you resolved these issues??

Hi,
Manual Review
1. Getting 404 error on clicking following links in publishthis setting page
Publishthis block setting
Publishthis curated content
Publishthis automated (feed) content
2. I am seeing this module under core package. Kindly move into corresponding package.

name = Publishthis
description = Publishthis Feed API Integration
package = Core
core = 7.x

Automated Review

There are number of issues reported by pareview. Please have a look at them and resolve them.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

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

No: Does not follow the guidelines for master branch. Please make sure you work on version specific branch. Eg. 7.x-1.x, or 7.x-2.x, etc for Drupal 7.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

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

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: Does not meet the security requirements. List of security issues identified:
The way you have added the JS code to the form is not ideal.
Eg. In get_bundle_auto_contents:

  $form['jssubmit'] = array(
      '#markup' => '<script>
      var loading_img = "' . $image_path . '";
      get_contents(' . $page . ',\'GetBundleAutoContents\');
      </script>',
    );
  

This could expose the module to security loophole, if the values returned in the variables $image_path, and $page could be manipulated.
Note: I could not test this due to the issues mentioned below. There is a possiblity of this being a false positive.
The ideal way to add JS to a form is using the #attached attribute.

Coding style & Drupal API usage

1. (*) You have used the PHP function include_once at the start of publishthis.module. The Drupal way of adding external files is by using the module_load_include function.
   The recommended way to include such files is to add them to the .info file as follows:

         files[] = publishthis.inc
         files[] = publishthis.api.inc
         

2. (+) In publishthis.inc file, the functions should be prefixed using the module name to avoid namespace conflicts.
3. (+) The menu item admin/config/content/publish/delete showed a WSOD. The following error messages were displayed on navigating to the other page:
   

   Notice: Undefined offset: -1 in _publishthis_node_confirm_delete() (line 135 of /Users/apple/www/contrib/sites/all/modules/sandbox/publishthis_feed_api/publishthis/publishthis.module).
   Notice: Undefined offset: 0 in _publishthis_node_confirm_delete() (line 158 of /Users/apple/www/contrib/sites/all/modules/sandbox/publishthis_feed_api/publishthis/publishthis.module).

4. (+) On cloning, the module directory structure was :
   

           publishthis_feed_api/
             publishthis/
               publishthis.module
               README.txt
             README.txt
         

   Fix the directory structure so that it looks something like the following:

           publishthis/
             publishthis.module
             README.txt
         

   Remove the duplicate README file.

5. (*) The implementation of hook_init is not required / reliable. The JS and CSS files could also be added using the .info file as mentioned above.
6. (*) After saving the configuration at admin/config/development/publishthis there should be a message displayed to the user (eg. "Your configurations have been saved"). I was not sure if anything happened when I first submitted the form.
7. (*) The URLs admin/config/development/curatedbundlecontent and admin/config/development/bundleautocontent displayed the following warning message with nothing else on the page:
   

   Warning: Creating default object from empty value in get_publish_thisdata() (line 42 of /Users/apple/www/contrib/sites/all/modules/sandbox/publishthis_feed_api/publishthis/publishthis.api.inc).

   I was not able to test the module further due to this.

8. (*) All the configuration menu items fall under admin/config/development. These could live under admin/config/services IMO because it makes service calls. Also, the menus could be displayed as tabs with a common parent, which makes it easy for the user to navigate between them.
9. (*) Configuration form of the block displayed the following error message:
   

   Warning: Creating default object from empty value in get_publish_thisdata() (line 42 of /Users/apple/www/contrib/sites/all/modules/sandbox/publishthis_feed_api/publishthis/publishthis.api.inc).

10. (*) There is no need to call drupal_install_schema inside the implementation of hook_install. Drupal installs the schema automatically. Similarly, drupal_uninstall_schema is also not necessary.
11. (*) In the table definition in hook_schema there are many TODO. Eg. TODO: please describe this table!.
    Please make sure you have correct description given to the table and columns.
12. (*) In hook_uninstall make sure you delete all the variables defined by the module.
13. You have defined the URLs and permissions defined by the module as PHP constants, eg. define("PUBLISHTHIS_URL", "admin/config/publishthis");. It left the module code hard to read and understand; I had to go to and fro from publishthis.inc where these constants are defined, and the other files where they have been used (mainly the module file). Most of these constants could be removed. Just a recommendation.
14. The function publishthis_as_block_settings_form could return system_settings_form, with appropriate variable name.
15. The function get_publish_thisdata makes curl request to the API. It is recommended to use drupal_http_request function to not have dependency on PHP curl. Note: This is not so important as guzzlehttp library in Drupal 8 uses CURL internally.

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.

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

This review uses the Project Application Review Template.

Also check for the issues mentioned by @ajalan065

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.