[1.0.x] Time Clock

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

Thanks @hemant-gupta for applying!
I have a few recommendations:
- /time_clock.libraries.yml - core/jquery.once: you add core/jquery.once as a dependency but never use it
- /js/timezone.js function ($, Drupal, once): You declare 'once' in the parameters but you don't use it (I think you can remove it)
- /css/custom.css: This file is empty (you can delete it)
- /time_clock.module time_clock_theme($existing, $theme, $type, $path): I think you can remove these unused params ($existing, $theme, $type, $path)
- /time_clock.module hook_help() function time_clock_help($route_name, RouteMatchInterface $route_match): You can remove $route_match if you don't use it

Thanks for your review @nikral!

I have changed the codes accordingly in DEV, please see.

I did not made a complete review, but I noticed the module implements a form class for the block settings. There is no need to implement a form class in that case. Block classes must implement the BlockPluginInterface interface which defines three methods for the block form: blockForm(), blockValidate(), and blockSubmit().

I have used a block built only to make its data available to users and not implemented the form block as not needed, kindly recheck this.

If a form block is not necessary, there should not be any form class, since the existing form class is used only for the block settings.

OK, thanks for valuable info.

I have removed the form class and implemented "BlockPluginInterface" for the block settings now, please check.

Some comments - this is not a full review.

1. Wouldn't it be nice to rely on the Drupal core date and time formats? Then this module does not need to worry about providing different formats and the users get more flexibility. Also, why not format the time and date entirely in the back end? This would make the JavaScript obsolete.
2. One could also rely on the users selected time zone or the default time zone of the site.
3. The PHPDoc of the TimeClockBlock constructor is incorrect.
4. Doesn't this block "suffer" from caching? Once the block is cached it would always display the cached date and time, correct?
5. The indentation in time-clock.html.twig is incorrect.

Thanks for your feedback!

Actually, this is a time centric module and if we use formats from 'admin/config/regional/date-time' then many date formats of them doesn't have a time to show and all these formats are freely managed in back-end on admin preference so I preferred like this way. Also, the block does not 'suffer' from caching to show us an updated content.

Suggested points has been corrected, let me know if still any thing found.

Tested the block and the initial date and time do get cached. Therefore until the JavaScript is loaded the block will display cached values. When the user does not have JavaScript enabled (unlikely), the block will show wrong values. See this blog article on lazy builders which could be interesting in that context.

Yes, I got your point and tried the same as they did in example but the module BigPipe does not support any more for the later version of Drupal 8 and that's why JS snippet covers this gap.

Bigpipe is now part of Drupal core. Lazy building works without Bigpipe. Bigpipe just changes the delivery strategy of the content.

Lazy builder added now. Let me know if further improvements needed.

FILE: time_clock.info.yml

package: Custom

That line is used by custom modules created for specific sites. It is not a package name used for projects hosted on drupal.org.

It's corrected, thanks!

Hi @hemant-gupta

Thx for your contribution! Good job and keep it up! I will leave some comments after the review:

• Check this → guide for more information on how to format the README
• You don't need the file LICENSE.txt
• The file js/timezone.js. It's not ok to declare functions inside the attach: of a Drupal behavior. Move them out of that behavior. Also, I am not sure about this statement today = today.toLocaleString("en-US", {timeZone:timezone, hour12:timeformat}); specially en-US
• The file templates/time-clock.html.twig. There is a missing doc block. Check this → guide and add the doxy to the twig file.
• The file src/Plugin/Block/TimeClockBlock.php. It's recommended to add the type of the property
  

  protected 
   DateFormatterInterface $dateFormatter;

  . Also, Are you sure Constructs a new WorkspaceSwitcherBlock instance.?

• The same file as above. This statement:
  

  if ($dateview) {
    $date = $this->dateFormatter->format(time(), 'custom', $date_format);
  }
  else {
    $date = '';
  }
  

  why not like this:

  $date = '';
  if ($dateview) {
    $date = $this->dateFormatter->format(time(), 'custom', $date_format);
  }
  

  . Besides,

• Same file as above. Please, add the doxy for the method generateTimestamp. No declared params in the description, no return type, no params type, etc.
• Same file as above. Why do you need these options wrapped in t function:
  

  $this->t('12/31/1980'),
  $this->t('31/12/1980'),
  $this->t('1980/12/31'),
  

  . I think there is no reason in that.

• File time_clock.module. A part of the $output variable from the hook time_clock_help has no t() wrap. Check the line 18.

Thanks for your feedback, @andrei.vesterli!

All suggested points have been addressed accordingly.

Actually, the file containing the license should be present in every project repository. It was not necessary when CVS was used because, essentially, all the project were hosted in a single repository which contained also Drupal core.

License file is added again! @apaderno

Good code, a clear README, Coder returns a couple errors in the phpunit test, PARreview looks good. I didn't see any noticeable security vulnerabilities. Get these last things fixed so I can get you pushed to RTBC. Thanks

FILE: /var/www/html/global/web/modules/contrib/time_clock/src/Form/TimeClockForm.php
-----------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------
7 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Config\ConfigFactoryInterface.
-----------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/global/web/modules/contrib/time_clock/src/Plugin/Block/TimeClockBlock.php
-----------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------
7 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Config\ConfigFactoryInterface.
-----------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------------

FILE: /var/www/html/global/web/modules/contrib/time_clock/js/timezone.js
---------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------
8 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "FALSE" but found "false"
8 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "TRUE" but found "true"
21 | ERROR | [x] Space before opening parenthesis of function call prohibited
21 | ERROR | [x] Expected 1 newline at end of file; 0 found
---------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------

Time: 2.93 secs; Memory: 10MB

Since he is passing you to needs review if no one objects your good with me.

Thanks for your review! @trigve-hagen
Also, your points are closed in DEV branch.

Thank you for your contribution!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.

These are some recommended readings to help you with maintainership:

• Dries → ' post on Responsible maintainers
• Maintainership →
• Git version control system →
• Issue procedures and etiquette →
• Maintaining and responding to issues for a project →
• Release naming conventions → .

You can find more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !

Thank you 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.

I thank also the dedicated reviewers as well.

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