[D7]Video Schema

Hello, first you should run all the automatic reviews with http://pareview.sh

Why are you creating a new content type and many fields to store your video. Maybe you should add media and media video modules and add alter functions to modifiy them.

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.

Ok i understand.
You are adding a content type with data to create a schema for a video. But this video will be a media video or something else and not provided by your module and could reference your schema with entity reference for instance, just like profile2 for a user.

i saw in code that you are creating a block defined as appearing on every page :

function video_schema_block_info() {
  $blocks = array();
  $blocks['video_schema_block'] = array(
    'info' => t('Video Schema Data'),
    'status'     => TRUE,
    'region'     => 'content',
    'visibility' => BLOCK_VISIBILITY_NOTLISTED,
  );
  return $blocks;
}

and then in hook_block_view() you are checking if the current path is a video node path :

function _video_schema_block_video_data() {
  $data = NULL;
  $cur_url_string = $_GET['q'];
  $complete_url = explode('/', $cur_url_string);
  // print_r($complete_url);
  // exit();
  if (drupal_is_front_page()) {
  }
  else {
    if (isset($complete_url[1]) && !empty($complete_url[1])) {

      if (isset($complete_url[0]) && !empty($complete_url[0]) && $complete_url[0] == 'node') {
       ...
      }
    }
  }
}

Maybe you should consider not to display your data as a block but alter the video node view with hook_node_view() instead.
Your are excluding the front page but what if the front page is a actually a video page ?

Hello sourabhutani,

you missed a lot of code commented, like

// print_r($videodata);

// $comma= '';

// Message drupal_set_message(t('video data submitted successfully'));
// Echo for formatting echo "<pre>";
// print_r($form_state['values']);
// exit();

you need to cleanup your code and write some real inline comments

Hey sourabhutani,

As reference to your above comment, still there is commented code. Is 'weight' need in future? if not please remove.

function video_schema_node_view($node, $view_mode = 'full', $langcode = NULL) {
if ($node->type == 'video') {
$node->content['schema_field'] = array(
'#markup' => _video_schema_block_video_data(),
// '#weight' => 10, .
);

}

return $node;
}

Regards,

Hi @sourabhutani,

I have manually reviewed your code. I have found the below issues.

(*) Please check if $_GET['q'] contains Drupal's node url i.e. "node/[nid]" and also check if $complete_url['1'] is numeric. Else node load function will result in fatal error.

function _video_schema_block_video_data() {
  $data = NULL;
  $cur_url_string = $_GET['q'];
  $complete_url = explode('/', $cur_url_string);
  $cur_node = node_load($complete_url[1]);
  if (isset($cur_node->nid) && !empty($cur_node->nid)) {
    $data = video_schema_getproperdata($cur_node->nid);
  }

  return $data;
}

(*) The data entered by user should be passed through check_plain() or filter_xss() functions to sanitize the data before submitting to database.

function video_schema_data_submit(&$form, &$form_state) {
  $nid = $form_state['node']->nid;
  $ddm = array(

    'video_name' => $form_state['values']['video_schema_video_name'][LANGUAGE_NONE]['0']['value'],
    'video_desc' => $form_state['values']['video_schema_video_desc'][LANGUAGE_NONE]['0']['value'],
    'video_thumb_url' => $form_state['values']['video_schema_video_thumb_url'][LANGUAGE_NONE]['0']['value'],
    'video_content_url' => $form_state['values']['video_schema_video_content_url'][LANGUAGE_NONE]['0']['value'],
    'video_embed_url' => $form_state['values']['video_schema_video_embed_url'][LANGUAGE_NONE]['0']['value'],
    'video_upload_date' => $form_state['values']['video_schema_video_upload_date'][LANGUAGE_NONE]['0']['value'],
    'video_duration' => $form_state['values']['video_schema_video_duration'][LANGUAGE_NONE]['0']['value'],

  );

  $data = drupal_json_encode($ddm);
  db_merge('video_schema')
       ->key(array('nid' => $nid))
       ->fields(array(
         'nid' => $nid,
         'data' => $data,
       ))
       ->execute();
}

Changing the status to "Needs work". Please fix the above issues and change the status back to "Needs review".

Changing the status to "Needs work".

Thanks for your contribution!!

@th_tushar: no, data should NOT be sanitized when saved to the database: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984

Make sure to read that article again.

Hi @sourabhutani,

Ignoring #14, as per klausi comment.

I have manually reviewed your code. I have found the below issues.

(*) Please check if $_GET['q'] contains Drupal's node url i.e. "node/[nid]" and also check if $complete_url['1'] is numeric. Else node load function will result in fatal error.

function _video_schema_block_video_data() {
  $data = NULL;
  $cur_url_string = $_GET['q'];
  $complete_url = explode('/', $cur_url_string);
  $cur_node = node_load($complete_url[1]);
  if (isset($cur_node->nid) && !empty($cur_node->nid)) {
    $data = video_schema_getproperdata($cur_node->nid);
  }

  return $data;
}

Changing the status to "Needs work". Please fix the above issues and change the status back to "Needs review".

Hi @sourabhutani,

1. It is prefer-able to use hook_node_info() to create a content type.
2. All module variables should be deleted on module un-installation.
3. The values of the fields are stored in the field tables. I think custom submit handler and custom table will not be required, which will also eliminate the hook_form_alter() function.
4. A new content type is being created for the display of video. Can it be a field that can be attached to any content type that user needs?

Changing the status to "Needs work". Please fix the above issues and update the status back to "Needs review".

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.

Hi

I found you have changed your concept from creating custom content type to field type. That is a good approach. In my case schema is creating when i set up multiple values to 1. When i increased multiple values http://screencast.com/t/2z0XWFipUf7 to greated than 1. It doesn't assume other fields and create schema for first field. Please have a look.

Thanks!

Review of the 7.x-1.x branch (commit 22372b8):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/video_schema.module
  --------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  --------------------------------------------------------------------------
   124 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 10
   126 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 10
  --------------------------------------------------------------------------
  PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. video_schema_field_schema(): instead of datetime we usually use unix timestamps as integer in Drupal for easier date handling. Any reason why you use datetime here? Please add a comment.
2. video_schema_field_formatter_view(): you are calling theme() with a theme hook that does not exist? Did you forget to implement hook_theme()? See https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
3. video_schema_node_view(): this hooks i now not needed anymore and you should do all your display stuff in video_schema_field_formatter_view() by implementing a proper theme function.
4. This module contains a security vulnerability and as part of our git admin training I'm assigning this to heykarthikwithu so that he can take a look. If he does not find anything I'm going to post the vulnerability details in one week. 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.

Yes, this is having a cross-site scripting (XSS) vulnerability.

After adding the Video schema field type to a content type and while adding the node of that particular content type, if we place the javascript in the video scheme field. And save the node.
We can see the javascript code is been injected, on viewing the node.

@klausi, assigning back to you, to have a check on my review.

That's right, it is also a good idea to tell the applicants where the vulnerability should be fixed.

video_schema_getproperdata(): user provided text must be sanitized before printing to HTML. Make sure to read https://www.drupal.org/node/28984 again.

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.