[D7] TypeIt

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

Automated Review

Please fix reported errors on https://pareview.sh/node/103

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

[No: Does not follow] the guidelines for 3rd party assets/code.

I am not sure, regarding this. @klausi : Please correct me if i am wrong here.
I find following text in typeit_main.js

/**
 * jQuery TypeIt
 * @author Alex MacArthur (http://macarthur.me)
 * @version 4.2.3
 * @copyright 2016 Alex MacArthur
 * @description Types out a given string or strings.
 */

 /* jslint browser: true */
 /* globals jQuery:false */

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows / No: Does not follow] the guidelines for project length and complexity.

Secure code

[ No: List of security issues identified.]

1: Anyone with the "access content" permission can access the configuration page . Its not a realistic scenario, so you should use a proper permission.

function typeit_menu() {
    
  $items['admin/config/user-interface/typeit'] = array(
  'title' => 'Typeit Settings',
  'description' => 'Change typeit module setting from here.',
  'page callback' => 'drupal_get_form',
  'page arguments' => array('typeit_add_more'),
  'access callback' => TRUE,
  'access arguments' => array('access content'),
  'type' => MENU_NORMAL_ITEM
  );
    
  return $items;
}

2: You need to sanitize user provided input before printing. You can test it by inserting the following value:
;} /*]]>*/--> </style><script>alert('hacked')</script>
After saving, a popup will be visible with block.
For more information about sanitizing, please read https://www.drupal.org/node/28984.

function _typeit_content() {
    
  $all_data_from_typeit_table = db_query('SELECT typeit_text FROM {typeit_data}')->fetchAll();
  $my_data = array();
  foreach ($all_data_from_typeit_table as $data_for_rows) {
    array_push($my_data, $data_for_rows->typeit_text);
  }

  /* Store all values in output */
  $output = "<div id='typeit-data'>";
  if (!empty($my_data)) {
    foreach ($my_data as $output_data) {
        $output .= "<p>" . $output_data . "</p>";    
    }
  }
  $output .= "</div><div id='typeit-print-data'></div>";    

  return $output;
}

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Please delete all variables used in module on hook_uninstall().

Example :
variable_set("typeit_variable", $all_dynamic_value);

2. (*) Getting Notice/Warning while trying to save configuration page. Please find the screenshot

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.

Hi,
Below are my review points:

1) Please follow README template..
2) Use t() for form description field.
3) Please implement hook_help

suggestion

better to use hook_theme for _typeit_content() return value.

Hi akhilsoni1992,

I'd made a little code review.

• Title and description should not enclosed with t() in hook_menu()
• Options key value should not be enclosed with t().
  

  /* Radio buttons for js values  */
    $form['javascript_values']['loop'] = array(
      '#type' => 'radios',
      '#title' => t('Do you want to loop your Typeit Text?'),
      '#options' => array(t('FALSE'), t('TRUE')),
      '#default_value' => $value_in_time_intervel['loop_data_for_typit'],
    );
  

• Instead of $_POST use $form_state to get form values in typeit_add_more_submit().
• Don't use HTML code inside PHP tag use theme function or create template file
• Remove hook_install function because it is not in use

Regards,
Sanjay Chauhan

I wanted to try this out for you but I really don't know what to expect.
The only information on this, and on your project page, is:

> This module is used to display typeit js effect.
> This module is developed successfully you can use it for typeit effect.

So, all your instructions for :

* what this does
* what advantage this adds to our sites,
* how it works
* How to configure it
* why I want to use it
* how to use it
* how to test it

equals:

* This module is used to {unknown}.

:-/

If you want user, contributors and supporters to join in with your open-source contribution, it helps a lot if you can provide help and encouragement about what it is you are adding, and why anyone would like to touch this at all.

Screenshots on the project page are a good start - but frankly, I have no idea what the context is or what it is telling me! What does this say?
LINKS to whatever this is, maybe some READMEs, maybe a reference to the library documentation or a use-case would help.

If we have to do lots of homework to even figure out why this module exists - then it is likely to be skipped over, so we can instead test some other module that is testable.

(I'm trying to help - adding good documentation and "sizzle" is a very important part of being able to maintain a popular open-source contribution!)

Please review your code on "https://pareview.sh" site. We found some issue in your code. Here I have attached the link for your error code.

https://pareview.sh/node/103

Hi rajveergang
Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Oh dear. Oh no, this is not good.

Now we can see that this module is a wrapper around:
TypeIt: The Most Versatile jQuery Animated Typing Plugin on the Planet

And we can also see that the Code committed to the repository is a direct copy of code written and published by alexmacarthur

With his copyright removed!

And even worse, the licensing that this jquery plugin is offered under is not even GPL, or a permissive "GPL compatible" but a paid license - requested (on trust) by the author. By taking this code, and not even providing a link back as credit you are taking away his ability to receive payment for his work!

This module application violates that trust, it violates copyright law, and it violates at least two points of the Drupal Git Repository Usage policy that you agreed to when signing up on Drupal.org!

This is a hard fail, and the project application cannot proceed in this form.
You must immediately remove the unlicensed code from the repository, or this may become a matter for the Drupal Licensing Working Group

This must be deleted now!

I don't want to sound mean here, but may of us on drupal.org make our careers built on intellectual property one way or another, and we take licensing laws - including and especially the Open-Source licensing laws - extremely seriously - as well as correctly respecting and attributing the work of others in the way they wish.

-----------------------

There are ways - legal, proper and accepted ways - to build and contribute a module that acts as a wrapper around freely available third-party code like javascripts.
Briefly, this usually involves :
* providing instructions for the user to download and install the third party code themselves. Usually in a README and the project page.
* using the Drupal 'libraries' API to register, check and load the library as needed.

This issue really should be closed immediately, BUT we can use it as a learning experience - you have a chance to rewrite it and use the third-party download process the proper way. And demonstrate that you understand why stealing someone else's code and cutting their name off the top is unacceptable.

Please have a look at any of the existing modules that do this already and the Libraries API docs there.

It's great that you may have permission from the author to use this library - that's a good step in co-operation.

However, you don't seem to understand the importance of the official Drupal policy regarding 3rd party libraries and content on Drupal.org we all agreed to. Even with explicit permission, or 'copyleft/free' licenses, drupal.org doesn't host GPL-"compatible" code.

You still need to look at the process used by all other Drupal modules that use external javascript libraries, and provide *instructions* for redeploying, not a copy & paste of the code. Instructions above at the bottom of #32 must be followed.

Hi, I am Gisle Hannemyr of the Drupal Licensing Working Group (LWG). Our job is to enforce the Drupal Git Repository Usage policy.

When you requested permission to have your project in the Drupal Git Repository, you promised to adhere to this policy. Now, you've broken that promise and US copyright law by including a copy of The TypeIt in your Git repo at Drupal.org:

1. This code is third party code. which is not allowed.
2. This code also has a commercial license and can not be licensed under GNU/GPL version 2 and later by you.
3. You've also violated US copyright law by removing the original author's copyright notice and link to website from his code.

Unless the original author creates a version licensed under GNU/GPL version 2 and later on his own website, the project in question can not be hosted on Drupal.org at all. (If and when he does, you can make provisions in the README for having it installed as a third party library as suggested by dman in #32.)

You should also re-read the Drupal Git Repository Usage policy, including the section with the heading "Termination".

These are serious policy violations. You must withdraw your project application immediately, and delete the repository with the stolen code from Drupal.org.

After you've withdrawn from the application process and deleted the repository with the stolen code, you may reapply. If you want to try again, make sure that there is no third party code in your aplication.

This license violation is now tracked by the LWG: #2832960: "TypeIt" project application - copy & paste of a non-free javascript library-

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

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

@Akhilsoni: Please update dependencies of libraries module in .info file because if we going to enable this module without libraries module, Its show following error.
"Fatal error: Call to undefined function libraries_get_path() in /var/www/html/drupal7-1/sites/all/modules/issues/typeit/typeit.module on line 50"
Add following line in typeit.info file.
dependencies[] = libraries

Please do not assign ticket yourself.
See the workflow https://www.drupal.org/node/532400

This was accidentally moved to the wrong issue queue.

Update the issue's summary. It's little hard to do... Please use Headings and Subheading (H2, H3, H4 tags) to separate different paragraphs into the project's introduction.

The Github repository dman referred in the previous comment is https://github.com/alexmacarthur/typeit. It seems the license is reported to be GPLv2, but the documentation is a bit vague.

Personal or Open Source - Just take it! It’s free to use, as long as the project remains personal or open source.

The jQuery plugin is not in Drupal.org repository, anyway. The license is not anymore an issue for us.

I agree that licensing is no longer a blocker.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
• Not all the points are application stoppers; some of them describe changes that would be preferable to make

/**
 * Implements hook_menu().
 */
function typeit_menu() {

  $items['admin/config/user-interface/typeit'] = array(
    'title' => 'Typeit Settings',
    'description' => 'Change typeit module setting from here.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('typeit_add_more'),
    'access callback' => 'user_is_logged_in',
    'type' => MENU_NORMAL_ITEM,
  );

  return $items;
}

// Attache Js.
if (variable_get('typeit_js_source') == 1) {
  drupal_add_js('https://cdn.jsdelivr.net/jquery.typeit/4.3.0/typeit.min.js');
}
else {
  if (libraries_get_path('typeit') == TRUE) {
    drupal_add_js(libraries_get_path('typeit') . '/typeit_main.js', array('preprocess' => TRUE));
  }
  else {
    drupal_set_message(t('Please download the source of typeit js from <a href="https://github.com/alexmacarthur/typeit" target="_blank"> Typeit </a>'), 'warning');
  }
}
drupal_add_js(drupal_get_path('module', 'typeit') . "/js/typeit.js");

There is code outside functions that needs to be removed or placed in a function.

/**
 * Implements hook_menu().
 */
function typeit_menu() {

  $items['admin/config/user-interface/typeit'] = array(
    'title' => 'Typeit Settings',
    'description' => 'Change typeit module setting from here.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('typeit_add_more'),
    'access callback' => 'user_is_logged_in',
    'type' => MENU_NORMAL_ITEM,
  );

  return $items;
}

The access callback needs to be more specific, or the settings form will be accessible to every user with an account on the site.

/**
 * Form Validation og typeit.
 */
function typeit_add_more_validate($form, &$form_state) {

  $numeric_values_only = $_POST['javascript_values']['time_intervel'];
  $delete_speed_value_check = $_POST['javascript_values']['deletespeed'];
  $cursor_speed_value_check = $_POST['javascript_values']['cursorspeed'];
  if (!is_numeric($numeric_values_only)) {
    drupal_set_message(t("Enter numeric value for Time intervel"), "error");
  }
  if (!is_numeric($delete_speed_value_check)) {
    drupal_set_message(t("Enter numeric value for Delete speed"), "error");
  }
  if (!is_numeric($cursor_speed_value_check)) {
    drupal_set_message(t("Enter numeric value for Cursor speed"), "error");
  }
}

Form validation handlers and form submission handlers should use $form_state to get the submitted values.

There are some typos in the text shown to users, for example intervel instead of interval.

Please don't remove the PAReview: security tag. We keep that for statistics and to show examples of security problems.

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.