[D7] Text to HTML List

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

issues found.
1. /root/repos/pareviewsh/pareview_temp/js/text_to_list.js: line 1, col 2, Error - Use the function form of 'use strict'. (strict)
2. /root/repos/pareviewsh/pareview_temp/js/text_to_list.js: line 3, col 1, Error - Expected indentation of 2 spaces but found 0. (indent)
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
Yes: Follows the guidelines for 3rd party assets/code.
README.txt
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.
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:

It is good to put drupal template file in template directory.

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.

@varghese: The 2 JS errors and the template folder is surely not an application blocker. Anything else that you found or should this be RTBC instead?

This is I tested from the automated tool. Please check the below url.

https://pareview.sh/node/312

Installed and enabled the module on a vanilla Drupal 7 site.

A couple of issues I see:

1. The project (folder) name is different from the module name (text_to_html_list vs text_to_list). Not a killer but annoying.
2. When first adding a text to list field, I get a PHP notice regarding an undefined index for "list_type" on line 40 in text_to_list.module
3. I'd suggest using a combination of help text and css (specifically the cursor property) to indicate that the list in the preview pane is draggable and that the X will delete the item.
4. Dragging also adds a style attribute to the final markup. I'd strip that out, just in case.

Hi,

I have installed and enabled the module on a Drupal 7 site. Check the below issue

Issue:

If I enter

as text field than I will get a javascript popup on this page. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again. Reffer screen shots.

I am adding security tag as per https://www.drupal.org/node/2833485#comment-11818518 by muthukumar sri, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi ethan.han777,

I am listing down some functionality issues.

1. The list that I have entered is not persistent, after inserting data, let say I forget to add data in some mandatory field, the data in the Text to HTML List gets deleted. Same case in existing nodes. PFA screenshot for more details
2. After saving node, unable to view the data I have saved on node view page - this is essential
3. After changing the List type the existing data does not change

Below are my reviews:

used $markup variables two times , remove it,
code should like

line 183:
$markup = text_to_html_list_strip_html_attr(filter_xss($item['value'], $allowed_tags), 'class');

line 128:
drupal_add_css('https://opensource.keycdn.com/fontawesome/4.7.0/font-awesome.min.css', array('type' => 'external'));

Please implement hook_help on the top of .module

I have manually review your module. It is nice.
Few things that not major but nice to have.
1) The user can insert 2) No configuration option, for example the source of fontawesome. What happen if i already have the fontawesome css?

@ethan.han777 Nice Work.

Please Implement hook_help() .

@amit: looks like you forgot to change the status. Anything else that you found or should this be set to RTBC?

File: text_to_html_list.tpl.php
Lines: 21 & 26:
I believe the text "Preview" and "List items are draggable to change order." should be run through t().

File: text_to_html_list.module
Line 128:
$fontawesome_cdn = 'https://opensource.keycdn.com/fontawesome/4.7.0/font-awesome.min.css';
Are external libraries allowed? I'm not sure if this is any kind of deal-breaker or not. We probably want to ask someone official like @klausi or @kiamlaluno.

The missing t() on lines 21 & 26 are the only security issues I see, (I'll let someone else weigh in on the fontawesome library if they so choose).

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.