- ๐ฎ๐นItaly apaderno Brescia, ๐ฎ๐น
The issue summary should always describe what the issue is trying to fix and, in the case, of coding standards issues, show which command has been used, which arguments have been used, and which report that command shown.
- Status changed to Needs review
about 1 year ago 6:54am 12 May 2023 - ๐ฎ๐ณIndia Ashutosh Ahirwal India
Providing patch with fixes.
Please review. - First commit to issue fork.
- ๐ฎ๐ณIndia dineshkumarbollu
Hi
The patch#6 applied cleanly with no phpcs errors.
Moving +RTBC
- Status changed to RTBC
about 1 year ago 7:50am 12 May 2023 - Status changed to Needs work
about 1 year ago 9:26am 12 May 2023 - ๐ฎ๐นItaly apaderno Brescia, ๐ฎ๐น
+/** + * @file + * Provide some extra functionality for module. + */
The usual description is Hook implementations for the [module name] module. (Indeed, [module name] must be replaced by the module name.)
+ /** + * The module handler service. + * + * @var \Drupal\Core\Extension\ModuleHandlerInterface + */
There is no need to say it is a service.
+ /** + * The cryptography service. + * + * @var \Drupal\Component\Utility\CryptoService + */ protected CryptoService $cryptoService;
Drupal core does not have any
\Drupal\Component\Utility\CryptoService
class.+ /** + * {@inheritdoc} + */ public function __construct(string $root, ModuleHandlerInterface $moduleHandler, AccountInterface $currentUser, CsrfTokenGenerator $csrfToken, LoggerChannelInterface $logger, CryptoService $cryptoService) {
{@inheritdoc}
is not used for constructors.
+/** + * Class to providing functionality SessionlessFormsServiceProvider. + */
Class descriptions must not start with Class not repeat the class name.
That description does not make sense either.+ /** + * The sticky query storage implementation used for storing sticky queries. + * + * @var \Drupal\sticky_query\StickyQueryStorage\StickyQueryStorageInterface + */ protected StickyQueryStorageInterface $storage;
What is a sticky query storage?
/** - * Key Storage + * Key Storage. * * @internal *
The definite article is missing in the short description.
+ /** + * The key storage object for this class. + * + * @var \Drupal\Core\Encryption\CryptoKeyStorage + */ + protected $keyStorage;
There is no need to say for this class: Every property is for the class that defines it.
- /** @noinspection PhpUnnecessaryLocalVariableInspection */ + /* @noinspection PhpUnnecessaryLocalVariableInspection */
That line is already correct as it is.
+ /** + * {@inheritdoc} + */ protected static function getCryptoService(): CryptoService {
{@inheritdoc}
cannot be used for traits that do not inherit from other traits. - Assigned to nitin_lama
- @nitin_lama opened merge request.
- Issue was unassigned.
- Status changed to Needs review
8 months ago 11:25am 20 October 2023 - Status changed to Needs work
8 months ago 1:11pm 20 October 2023 - ๐ฎ๐นItaly apaderno Brescia, ๐ฎ๐น
+/** + * @file + * Sessionless Forms module for encrypting and signing form data. + */
The usual description is Hook implementations for the [module name] module. where [module name] is the module name shown in the .info.yml file.
+ /** + * The module handler service. + * + * @var \Drupal\Core\Extension\ModuleHandlerInterface + */
+ /** + * The CSRF token generator service. + * + * @var \Drupal\Core\Access\CsrfTokenGenerator + */ protected CsrfTokenGenerator $csrfToken; + /** + * The logger channel service. + * + * @var \Drupal\Core\Logger\LoggerChannelInterface + */ protected LoggerChannelInterface $logger;
There is no need to say service in the descriptions.
+ /** + * The CryptoService for handling encryption and signing. + * + * @var \Drupal\sessionless\CryptoService + */ protected CryptoService $cryptoService;
The description must not include the class name.
+ /** + * Get cache. + * + * Retrieves the cached form data from encrypted fields. + * + * @param string $form_build_id + * The form build ID. + * @param \Drupal\Core\Form\FormStateInterface $form_state + * The form state. + * + * @return mixed + * The cached form. + */
The verb in the short description must be declined to the third person singular.
I am not sure the long description is correct, since the cached data is not retrieved from the encrypted fields. It is retrieved from the cache.+ /** + * Delete cache. + * + * Deletes cached form data. + * + * @param string $form_build_id + * The form build ID to identify the cached data.
There is no need to use short and long description, when the long description is an one-line description.
+/** + * Service provider for Sessionless Forms module. + */ class SessionlessFormsServiceProvider extends ServiceProviderBase {
The description must not include the class name. It is also too generic.
+ /** + * The storage for session data. + * + * @var \Drupal\sticky_query\StickyQueryStorage\StickyQueryStorageInterface + */ protected StickyQueryStorageInterface $storage;
It does not seem that is the session data storage.
+ /** + * SessionlessSession constructor. + * + * @param \Drupal\sticky_query\StickyQueryStorage\StickyQueryStorageInterface $storage + * The storage for session data. + * @param bool $encrypt + * (Optional) TRUE to encrypt session data, FALSE to skip encryption. + */ public function __construct(StickyQueryStorageInterface $storage, bool $encrypt = TRUE) {
The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.
+ /** + * Start the session. + */ public function start() {} + /** + * Get the session ID. + * + * @return string + * The session ID. + */ public function getId() { return 'sessionless'; } + /** + * Set the session ID. + * + * @param string $id + * The session ID. + */
The verb in the description must be declined to the third person singular.
I would also check which of those methods are defined in an interface, as in that case the documentation comments are different.
+ * @return \BadMethodCallException + * An exception indicating the method is not allowed.
Thrown exceptions are documented with
@throws
, not@return
./** - * Key Storage + * Key Storage.
Only the first word in the description is capitalized. A definite article is missing.
+ /** + * The state service for managing key storage. + * + * @var \Drupal\Core\State\StateInterface + */ protected StateInterface $state;
The usual description of that service is The state key value store.
- First commit to issue fork.
- Assigned to imustakim
- Issue was unassigned.
- Status changed to Needs review
8 months ago 4:14am 24 October 2023