[D7] schemadata

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

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.

Adding [D7] to title.

Hi rajveergang,

1) In schemadata.module under function schemadata_show_rows() you should use t() function at line 142.

Thanks
Rishabh

Hi rajveergang,

Please fix below minor changes :

1) Please fix the spelling of "schemadata" from all the instances in the module file, you have written this as "scemadata", for example : "This shows all the scemadata" this string in all the hook_menu items.

2) Access arguments should be a string and it is consider as a good practice as you have written "administer_scemadata" and it is looks like a function name you should use this as "administer schemadata" and also fix the spelling in it as well.

Thanks,

Hi Rajveer,

You need to check the following points.

1. Use t() for return "table not found";
2. Check the spelling of $sortable_filed .
3. Give proper name to $temp_array variable.
4. Use global $base_url; first in the function.
5. "scemadata" typo should be "schemadata".
6. I would suggest url should be more clean / readable schemadata/allschema must be schema-data/all-schemas .
7. Check the sentence "Shows you schema's data" .
8. Check your readme file, Installation section needs change. it should be
"Install as you would normally install a contributed Drupal module. See:
https://drupal.org/documentation/install/modules-themes/modules-7
for further information.".

Hi Rajveer

1. Hook_help is missing.
2. The Readme file must be a bit more descriptive.
3. I would suggest to simply display the list of tables. Remove unnecessary logic in schemadata_show_all() to display in table format with rows containing 6 records each. You can simply display in list.
4. The module contains spell errors "scemadata" in schemadata_permission().
5. In your comment you have "mysl" change it to mysql at line no 7 of .module file.
6. The lines
"// Get all schema name.
$schema = drupal_get_schema(NULL, TRUE);
// Sort schema.
ksort($schema);
// Get table name.
$tables = array_keys($schema);"
are written twice. You can reduce them by creating a function "getSchema" to return the schema array in sorted format.
7. $output variable is not required. You can return directly.

Suggestions are upto you to implement it.

Thanks

Hi rajveergang,

Please fix these small things:

In hook_help:
1) Use para tag for new lines instead of multiple "br" in a single para tag. For ref: you can see admin_menu.module.

2) And check for spelling in your help text : localsystem should be local system OR local machine.

Thanks

Just few things more.

Move the hook_help to the top. This would keep your module code in proper order.
You are showing "table not found" and in my opinion it must be drupal access denied as the url is malformed.

Thanks

- missing newline at the end of the module file

- docblocks are not correctly formed, eg:

/*
* Implements hook_help()
*/

Please compare with core modules. I suggest using Module Builder to generate hook implementations for you!

- one of your menu items has no callback:

  $items['admin/config/content/schema-data'] = array(
    'title' => 'Show Schema Actions',
    'description' => 'Display all the functionality link under schemadata module',
    'access arguments' => array('administer schemadata'),
    'type' => MENU_NORMAL_ITEM,
  );

- you're not sanitizing output, so if you look at tables that contain user input, there is a security risk:

        $refined_data_rows[] = $data;

- you've got 3 variables called row something! It's a bit confusing.

    $refine_rows = array();
    foreach ($rows as $row) {
      $refined_data_rows = array();
      foreach ($row as $data) {
        $refined_data_rows[] = $data;
      }
      $refine_rows[] = $refined_data_rows;
    }

Instead of using fetchAll(), you can iterate over your DB result in the foreach.

> sanitizing output , as of now its not required here , because developers will use this module, permission is given to module

No, sanitizing output is definitely required. See https://www.drupal.org/writing-secure-code:

No piece of user-submitted content should ever be placed as-is into HTML.

You don't know which fields of which tables are user-submitted content. Certainly, everything in an entity base table or a field table.

Errmmm I don't think you should be setting your own project application to RTBC! You can however set it back to Needs Review once you've taken care of any issues raised by a reviewer.

looks like RTBC +1

looks like RTBC +1

@rajveergang Please find my comments below.

1. Please change the name in .info as human readable name.
2. Please update about where we can see the list of tables in readme.txt since it is not clear on how to reach that page unless we check the source code.
3. Please follow the README template.

Hello rajveergang,

Reviewers here have done great work ,, and you followed that so I'll do a review to see if this is a RTBC.

Automated Review

None.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes:Does not causes] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

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

[Yes: Meets the security requirements. / No: List of security issues identified.]

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. In hook_help(). and your function schemadata_show_all() .. consider using t().
2. in your .module file you are using filter_xss which allows you to pass some allowed HTML tags which I don't see the case here .. if its not the case you can just use check_plain() in your variables.
3. you have a problem with clean URLs .. try to access your page admin/config/content/schema-data/all-schema with clean URLs not enabled .. then try to see any table .. just fix the path .. I see you are using l() which should give you the right path whiether its clean URL or not but in your case something is wrong and should be fixed.

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.

although I see no blockers here so I think this is a RTBC :)

Thank you

Great work ,, I pulled your work but I still have the same error when clean URL is disabled .. I've attached error it might help getting a closer look.

have a nice day :)

@rajveergang,
1. you can remove this unused variable.

function schemadata_show_rows($table_name) {
$fields = array();

2. And

 * You may want to disable Toolbar module, since its output clashes with
   Administration Menu.

i was not able to reproduce this..

Else all looks fine :)
RTBC +1

Hello,

I confirm the issue is fixed :)

I have promoted the project for you. Now you can create the release of the project.

Thanks for your contribution, rajveergang!

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.