[D8] Skillset Inview

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

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.

Hello, I have installed and tested your module. When I try to ыset Section Title in the module settings get this errors:
Notice: Undefined index: skill_item in skillset_inview_form_submit() (line 534 of C:\xampp\htdocs\drupal\sites\all\modules\skillset_inview\skillset_inview.module).
Warning: Invalid argument supplied for foreach() in skillset_inview_form_submit() (line 534 of C:\xampp\htdocs\drupal\sites\all\modules\skillset_inview\skillset_inview.module).

Hi @SKAUGHT,

I have manually reviewed the D7 version of your module,

There is a security vulnerability in your module,

$current .= '<li>' . $item->name . ' <span>' . $item->percent . '%</span></li>' . PHP_EOL;

$item->name in skillset_inview_add_form function of .module file should be passed through check_plain function.

Would review the code again to find more such issues. Changing the status to "Needs work".

Hi @SKAUGHT,

The user entered fields should be stored as is in the database. When user edits the field, he should get what he has entered. The value should be sanitized before displaying it back to user. So, please fix the issue as per comment #23.

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

Thanks.

manual review of the 8.x branch:

1. the drupal 8 branch shows quite a few coding standard errors: http://pareview.sh/pareview/httpgitdrupalorgsandboxskaught2692853git
2. Unescape.php: why is this extension needed? Please describe that on the doc block of the class. What is the use case?
3. SkillsetInviewPreviewController: don't use global functions like db_select() or render() or \Drupal in classes. Always inject your dependencies, see https://www.drupal.org/node/2133171 . Same in SkillsetInviewAddForm.
4. SkillsetInviewPreviewController: don't use t() in controillers, use $this->t() instead.
5. SkillsetInviewAddForm::submitForm(): the submitted skill name is user provided text, so you should use the "@" or "%" placeholder with drupal_set_message(). I think the "!" placeholder does not even existing anymore in Drupal 8, right?
6. "t('@color is not a valid hexadecimal color.', ['@color' => Html::escape($values[$item])]))": no need to escape the value here, the "@" placeholder with t() will already do that for you. Make sure to read https://www.drupal.org/node/28984 again.
7. SkillsetInviewColorForm: why do you need to call drupal_flush_all_caches() here? How can your module affect each and every cache? Please add a comment in the code?
8. SkillsetInviewColorForm: why is there a script tag with document.write()? You can jsut include your text in the render array?
9. "'#title' => 'Inside Percent',": all user facing text must run through $this->t() for translation. Make sure to check all your strings.
10. Your classes like SkillsetInviewOrderForm are quite long because you repeat the module name in the class name. That is not necessary anymore in Drupal 8 because we have namespaces now anyway.
11. "t("Delete skill '@name'", ['@name' => trim(strip_tags(Html::normalize($row->name)))])": why is the strip_tags() necessary here? The "@placeholder will sanitize for you and just create an escaped version anyway. I think that whole "trim(strip_tags(Html::normalize()" wrapping can be removed.
12. SkillsetInviewOrderForm::alphaSort(): Do not use Xss::filter() here since you are not printing anything to HTML. "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

The wrong usage of t() is a blocker right now and you need to know where to apply sanitization and where not.

dependency injection: every time you are using \Drupal in a class you are doing it wrong. There are some exceptions, but most of the time it is possible to pass in services that you need through the constructor. https://www.drupal.org/node/2133171 has an intro on that and you can look at many examples in core that do it.

manual review:

1. SkillsetInviewAddForm: class comment is useless and only repeats the name. It should say what thew class doesm for example: "Provides a form to create a new skill item." or similar.
2. \Drupal::database() is wrong in a form class. Again, you are using the global \Drupal, but you should use somehting like $this->databseConnection() and inject that in the constructor. The documentation only suggests \Drupal for use in static functions. https://www.drupal.org/node/2203931 gives an example how to inject dependencies into a form. Please check that for all your form classes.
3. SkillsetInviewAddForm::submitForm(): still sanitizes the data to be saved to the database, which you should not do. It is perfectly fine to validate data in a validation handler, but once you are in a submission handler you should take the data as given and save it unaltered. Can you check all your submit handlers that they store the data unchanged in the database? Validation should be done in a validation handler.
4. SkillsetInviewColorForm: "Skillset Heading" all user facing text must run through $this->t() for translation.
5. "$this->t("Delete skill '@name'", ['@name' => trim(strip_tags(Html::normalize($row->name)))]),": why the strip_tags() here and the normalize call? Can you add a comment in the code why this is necessary or remove that?

But that are not critical application blockers, otherwise RTBC.

Assiging to mpdonadio as he might have time to take a final look at this.

Are we reviewing both the D7 and D8 branches, or just the D8 one?

And, I have read the chain here. Though you may not agree with it, save as-is and sanitize on output / use DB placeholders is the Drupal way of handling things, and is a perfectly acceptable from a security standpoint. Personally, I find this much easier and less error prone than scrubbing user input.

Automated Review

Review of the 8.x-1.x branch (commit 7878c40):

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

  /Users/matt/PAR/pareview_temp/js/skillset-inview-color.js: line 6, col 1, Error - Definition for rule 'keyword-spacing' was not found (keyword-spacing)
  /Users/matt/PAR/pareview_temp/js/skillset-inview-percent_assist.js: line 6, col 1, Error - Definition for rule 'keyword-spacing' was not found (keyword-spacing)
  /Users/matt/PAR/pareview_temp/js/skillset-inview.js: line 6, col 1, Error - Definition for rule 'keyword-spacing' was not found (keyword-spacing)
  
  3 problems
  

• 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.

FILE: /Users/matt/PAR/pareview_temp/skillset_inview.install
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 39 | ERROR | [x] Expected 1 blank line after function; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/skillset_inview.module
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
 22 | ERROR | [x] Expected 1 blank line after function; 2 found
 43 | ERROR | [x] Expected 1 blank line after function; 2 found
 51 | ERROR | [x] Expected 1 blank line after function; 2 found
 66 | ERROR | [x] Expected 1 blank line after function; 2 found
 83 | ERROR | [x] Expected 1 blank line after function; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/Form/SkillsetInviewAddForm.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  24 | ERROR | [x] Expected 1 blank line after function; 2 found
 112 | ERROR | [x] Expected 1 blank line after function; 2 found
 147 | ERROR | [x] Expected 1 blank line after function; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/Form/SkillsetInviewColorForm.php
------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------
 359 | ERROR | [x] Expected 1 blank line after function; 2 found
------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/Form/SkillsetInviewDeleteForm.php
-------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------
 82 | WARNING | Line exceeds 80 characters; contains 81 characters
-------------------------------------------------------------------------


FILE: /Users/matt/PAR/pareview_temp/src/Form/SkillsetInviewOrderForm.php
------------------------------------------------------------------------
FOUND 8 ERRORS AFFECTING 8 LINES
------------------------------------------------------------------------
 218 | ERROR | [x] Expected 1 blank line after function; 2 found
 226 | ERROR | [x] Expected 1 blank line after function; 2 found
 235 | ERROR | [x] Expected 1 blank line after function; 2 found
 243 | ERROR | [x] Expected 1 blank line after function; 2 found
 263 | ERROR | [x] Expected 1 blank line after function; 2 found
 284 | ERROR | [x] Expected 1 blank line after function; 2 found
 329 | ERROR | [x] Expected 1 blank line after function; 2 found
 370 | ERROR | [x] Expected 1 blank line after function; 2 found
------------------------------------------------------------------------
PHPCBF CAN FIX THE 8 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------

Time: 1.65 secs; Memory: 13.75Mb

Manual Review

CSS rule for '.skillset-inview-wrapper: hover' is broken.

In your OO code, inject your services and avoid the \Drupal static service container. This may be the best starting point, https://docs.acquia.com/articles/drupal-8-dependency-injection

The same goes for some of the static factory methods (like Url::fromRoute); there are services for nearly all of those.

Thought you shouldn't be using it in OO code, the convention is to explicitly use the static service container as ``\Drupal` and not `Drupal`.

Convention prefer that `use` blocks are alphabetical.

In render arrays, avoid concatenating markup with strings. See if there is a better render structure, or use #prefix and #suffix.

And, in a render array, avoid rendering things and using #markup. Just stuff them into the render array w/ the right properties.

The render array in SkillsetInviewPreviewController::preview() has a dependency on the user, so you need to add this cache context. Same with SkillsetInviewZero

Doxygen convention in Drupal is to fully qualify class names.

This has been beaten to death, but the Drupal convention is to store user input as-is and handle it properly on output and via database placeholders. Under old-policy this would be consideration for a blocking issue (major API problem), but no longer is.

You are building up an lot of markup on the fly. Consider moving a lot of this to Twig templates.

TBH, there are some head-scratchers security wise. The config variables are used as-is in but have validation.
This HTML is then passed to Markup::create(), which marks the string as safe and then stuffed into a #markup render element. #markup
normally sanitizes via Xss::filterAdmin(), but this is already a safe string so it isn't sanitized. I think these are OK b/c the validation, but I still think it would be
best to handle this complex markup via Twig (Markup really shouldn't be used this way). See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... for more info.

Why the full drupal_flush_all_caches() in the forms? Can't you just invalidate the proper render related things?

Typically, static:: is preferred over self:: (cf, PHP late static binding)

The Unescape class should not be needed w/ proper render arrays, theming, and Twig templates. You are just building up too much markup in PHP and not leveraging Drupal to do this for you.

I am not seeing any blocking issues here, but the comments above about Markup::create() and Unescape make me a little unseasy, but I don't see any exploitable vectors right now.

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.

Thanks for your contribution, SKAUGHT!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

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.

I am changing status to credit the users who participated on this issue and the duplicate ones. I apologize for bumping this application.