Description

Extra body classes is a simple module which will provide extra classes on the <body> tag.

Sandbox Project Page

https://www.drupal.org/sandbox/rahulbaisanemca/2583147

Git Instructions

git clone --branch 8.x-1.x http://git.drupal.org/sandbox/rahulbaisanemca/2583147.git extra_body_classes
cd extra_body_classes

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxrahulbaisanemca258314...

Manual reviews of other projects

https://www.drupal.org/node/2595865#comment-10464895
https://www.drupal.org/node/2594679#comment-10465153
https://www.drupal.org/node/2595919#comment-10465241

Comments

rahulbaisanemca created an issue. See original summary.

rahulbaisanemca’s picture

rahulbaisanemca commented
Title: Extra body classes » [d8] Extra body classes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
PA robot’s picture

PA robot commented

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.

rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Needs work

Thanks :) Add this tag "PAReview: review bonus" to your project application, you are eligible for the high priority list.

extra_body_classes.install:
- Use \Drupal::service('config.factory')->getEditable('extra_body_classes_date')->delete();. See https://www.drupal.org/node/2183531, https://www.drupal.org/node/1809490

extra_body_classes.module:
- I guess you checking whether current user is admin. You can do it using this:

global $user;
// Check whether $user has admin role.

Apart from the changes.. I am a bit confused about the scope of this module. What this module does is add some classes to body tag of page markup - if the user is admin. Some other observations:

  • The module description does not states this, you should consider specifying this condition. Also before doing this, consider this - a normal user would also like to add some class to the body markup. If the current page is admin related, I guess Drupal already provides this feature.
  • You are restricting the class to only date, what if the administrator wants it to be some other class.
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Issue tags: +PAreview: review bonus
ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Title: [d8] Extra body classes » [D8] Extra body classes
rahulbaisanemca’s picture

rahulbaisanemca commented
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented
Issue summary: View changes
rahulbaisanemca’s picture

rahulbaisanemca commented

Hi Subhojit, Thanks for reviewing my module.
I have come up with all suggestion u have suggested here... hope that correct (finger crossed).

1) extra_body_classes.install file is updated with given suggestion.
2) In extra_body_classes.module file i am checking whether current path comes under admin section, if not then only add extra classes.
for example: if path is /test then extra body class should be implement.
but if path is within admin section like /admin/modules then extra body classes should not implement.
3) There is very wide scope for this module there is lot of developer/site-builder/Themer right out there who wan't to date-based/current user role based/custom classes are needed.
4) according to your suggestion, now administrator can be able to add custom class to body tag

rahulbaisanemca’s picture

rahulbaisanemca commented
Status: Needs work » Needs review
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

- Consider adding comment in validateForm() for what you are trying to achieve.
- Change comment in submitForm() to @inheritdoc

rahulbaisanemca’s picture

rahulbaisanemca commented

Hi Subhojit,
Thanks for giving me ur time,
According to your suggestion, i have updated comments
1) comment in validateForm()
2) Change comment in submitForm() to @inheritdoc

Sumit kumar’s picture

Sumit kumar commented

Hi @rahulbaisanemca

Thanks for your contribution
Can you show help through Readme file,right now its coming from function that you have create in file.

 return t(" 
        <h3>CONTENTS OF THIS FILE</h3>
        ---------------------
        <ol>
         <li>Introduction</li>
         <li>Installation</li>
         <li>How to use</li>
         <li>Maintainers</li>
         <li>Supporting organization</li>
        </ol>
        <h3>INTRODUCTION</h3>
         * Extra body classes is a simple module which will provide extra classes on the <body> tag like day, month, year, current day time stamp, current user roles, custom classes. 
        <h3>INSTALLATION</h3>
         * Install it as you would normally install a contributed drupal module. See
           https://www.drupal.org/documentation/install/modules-themes/modules-8
        <ol><h3>HOW TO USE</h3>
         <li>After download place unzipped folder in  YourProjDir/modules/contrib  directory</li>
         <li>Go to admin/modules and enable this module.</li>
         <li>For configuration go to admin/config/content/extra-body-classes check options and save it</li>
        </ol>
        <h3>MAINTAINERS</h3>
        Current maintainers:
         * Rahul Baisane (rahulbaisanemca) - https://www.drupal.org/u/rahulbaisanemca
        <h3>SUPPORTING ORGANIZATION</h3>
        * QED42 Engineering Pvt. Ltd. (https://www.drupal.org/marketplace/qed42)
      ");

Thanks

rahulbaisanemca’s picture

rahulbaisanemca commented

Hi Sumit kumar,
Thanks for your review and time,
Here i am consider that u want me to write help into this format https://www.drupal.org/node/161085, like we write in d7.
For this i gone through core modules and example module and check how they have implemented hook_help.
They have implemented in a same way that i have implemented it.
May be i am not in sink with you, if so please provide, reference how would you like me to implement hook_help.

Sumit kumar’s picture

Sumit kumar commented

Hi @rahulbaisanemca

you can add file path in hook_help().
$filepath = dirname(__FILE__) . '/README.md';

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Needs work

Thanks @Sumit pointing that out. @rahulbaisanemca I totally missed out. @Sumit is right, your hook_help() needs to be updated. I am not sure whether this module's help would render properly, there may be escaping problem.

This is not the right way to render content for module help. Loading README file inside hook_help() will result in not-translated module help. You should refer webform module help. Also your module's help should focus on the purpose, installation and way of working of the module, credits should only be inside README.

rahulbaisanemca’s picture

rahulbaisanemca commented

Hi @Sumit and @Subhojit, I have updated hook_help and README.txt, Now all help text are written in t(), also now help is focus on Introduction, installation and how to use this module. Current maintainers: and Supporting Organisation are remove from help and make available only in README.txt

rahulbaisanemca’s picture

rahulbaisanemca commented
Status: Needs work » Needs review
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented

I am not sure whether this is properly rendered as link. Have you checked the help markup? You can use FormattableMarkup for rendering this. This is not a security issue, so I am not moving this to "needs work".

rahulbaisanemca’s picture

rahulbaisanemca commented

@subhojit, Thanks for pointing this issue, i have come up with updated hook_help code can u please review it.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Status: Needs review » Reviewed & tested by the community

Reviewed the code once again, didn't found any security concerns. I hope that this module comes up with more functionalities in future. Thanks for your contribution!

rahulbaisanemca’s picture

rahulbaisanemca commented

Sure subhojit, soon i will add new functionality to this module.

Thanks for ur time and review.

FreeAndEasy’s picture

FreeAndEasy commented
Status: Reviewed & tested by the community » Needs review

Automated Review

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

FILE: ...upal-7-pareview/pareview_temp/src/Form/ExtraBodyClassesConfigForm.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
27 | ERROR | Public method name "ExtraBodyClassesConfigForm::getFormID"
| | is not in lowerCamel format
---------------------------------------------------------------------------

Manual Review

Individual user account
Yes: Follows
No duplication
No. I think the same functionality will be provided by Display Suite in the future (If that is not already the case, its is in the D7 verison of Display Suite). https://www.drupal.org/project/ds
Master Branch
Yes: Follows
Licensing
Yes: Follows
3rd party assets/code
Yes: Follows
README.txt/README.md
Yes: Follows
Code long/complex enough for review
No: Does not follow the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements
Coding style & Drupal API usage

Follows.

This review uses the Project Application Review Template.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

@FreeAndEasy: module duplication is bad, but is not an application blocker. Looks like you did not find any problems in the code and this should stay RTBC?

FreeAndEasy’s picture

FreeAndEasy commented

@klausi: Jupp, other than maybe project complexity I have no further objections. I think the functionality this module provides is also already largely provided by Drupals Twig templates. One could easily use twigs date function in the html.html.twig template to achieve the same thing. A "logged_in" variable is also provided, the only thing missing is the role class (which could also be implemented via a theme hook).

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

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

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: ...les/contrib/pareview_temp/config/schema/extra_body_classes.schema.yml
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 24 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: .../www/drupal/modules/contrib/pareview_temp/extra_body_classes.info.yml
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 5 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...upal/modules/contrib/pareview_temp/extra_body_classes.permissions.yml
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 3 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...w/drupal/modules/contrib/pareview_temp/extra_body_classes.routing.yml
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 7 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: ...modules/contrib/pareview_temp/src/Form/ExtraBodyClassesConfigForm.php
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 27 | ERROR | Public method name "ExtraBodyClassesConfigForm::getFormID"
    |       | is not in lowerCamel format
---------------------------------------------------------------------------

Time: 155ms; Memory: 6.25Mb
  1. No need to explictly remove the config in extra_body_classes_uninstall. if you need default values of variables on install use config/install/extra_body_classes.settings.yml for it
  2. ExtraBodyClassesConfigForm : It would be better to inject the class rather than calling global \Drupal:: object . in buildForm you can access the config from $this->config b/c ConfigFormBase is injecting the config factory. See the ConfigFormBase for reference.
  3. ExtraBodyClassesConfigForm : validateForm line 103 please throw better UX message.
  4. submitForm : its better to get the config factory directly from the current object\Drupal::getContainer()->get('config.factory') $this->configFactory.
  5. extra_body_classes_preprocess_html : we are passing the classes array without sanitizing it. so please sanitize it before adding. the class names was unsanitized and yeah the classes are being validated in validate form function with the expression /^[a-zA-Z0-9,]*$/

Please fix the above and then do another 3 reviews to take another review bonus to speedup the process. Thanks for your patience involved in the project applications process.

rahulbaisanemca’s picture

rahulbaisanemca commented

Hi Naveen,

Thanks for your kind review, but i have just done this,
http://pareview.sh/pareview/httpgitdrupalorgsandboxrahulbaisanemca258314...
and it looks good for me can you please provide me link where you have tested this.

Thanks,
Rahul Baisane.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: -PAreview: security

Since the class name is validated in ExtraBodyClassesConfigForm I don't see how an attacker could use that to perform an XSS attack. And user roles can only be added by users that must be trusted anyway, so also the role name does not look like an attack vector to me. Maybe I'm missing something, could you post exploit details how this could be leveraged? Removing the security tag in the meantime.

rahulbaisanemca’s picture

rahulbaisanemca commented

Thanks klausi for this details.

naveenvalecha’s picture

naveenvalecha
He/Him
commented

Klausi,
Yeah Its fine with the class name. I did not tried to exploit it but the class names was unsanitized and yeah the classes are being validated with the expression /^[a-zA-Z0-9,]*$/
updated the comment above.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs work » Reviewed & tested by the community

Not any blocker above. Assigning to klausi to give it a final review when he will get time.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Assigned: Unassigned » klausi
rahulbaisanemca’s picture

rahulbaisanemca commented

Thanks @Naveen for your kind review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: klausi » Unassigned
Status: Reviewed & tested by the community » Fixed

The other points mentioned by Naveen are still valid, so make sure to check those.

Otherwise looks good to me, so ...

Thanks for your contribution, rahulbaisanemca!

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:

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.

rahulbaisanemca’s picture

rahulbaisanemca commented

Thanks klausi, This movement i am waiting for so long, finally...my module get accepted by community.
Feeling happy.
Thanks to Ajit for teaching me how to contribute, thanks for showing trust on me.
Thanks to Naveen for taking out time from your busy schedule and pushing this module one step further.
Thanks to Sumit kumar and FreeAndEasy for your time and review.
and Finally Big Thanks to Subhojit who is not only excellent Drupal Developer but also a wonderful human being.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.