[1.0.x] ONLYOFFICE

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

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 smother review.

To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .

Since the project is being used for this application, for the time this application is open, only the user who created the application can commit code.

Hello @onlyoffice,
Remove the develop branch

FILE: onlyoffice/src/OnlyofficeUrlHelper.php

    $linkParameters = [
      $file->uuid(),
      \Drupal::currentUser()->getAccount()->id(),
    ];

\Drupal calls should be avoided in classes, use dependency injection.

Thank you for applying!

Is the account used to create this application used by more than a person?

Hi vishal.kadam → , in source code Drupal are many places where this construction of calling the currentUser() method is used. We ran linters with the DrupalPractice standard and fixed all the places where there were warnings about calls to \Drupal ( https://www.drupal.org/project/onlyoffice/issues/3291611 📌 \Drupal calls should be avoided in classes, use dependency injection instead Fixed ). DrupalPractice allows the use of the \Drupal::currentUser() construct. Why can't we use it here?

phpcs does not report everything that should be changed; sometimes, it also reports false-positives. It cannot be used as proof the code is correct.

Thanks to all reviewers.
The develop branch has been removed.
The use of dependency injection is not required (by comment #9)

There have not been answers to comment #7.

Only one person has access to account ONLYOFFICE →

May you then add your first name and last name to the account?

No problem. Added.
It is planned that this is a corporate account. But at the moment it is run by one person.

An account that is planned to be used as corporate account cannot be used to make commits on drupal.org repositories. That is not allowed from the Drupal.org Terms of Service → .

3. If you are sharing your user account with multiple people (e.g. as your “official” organization account), you are not allowed to do the following using this account:

• Commit code to Git repositories on the Website
• Create any node except for organization, case study or project nodes
• Comment on nodes

Each developer needs to create their own account, which must not be shared with other people, in the present nor the future.

Ok. I understand
I added my first and last name. Changed the e-mail address to a personal one. Only I have access.
Thanks for the warning.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/Controller/OnlyofficeCallbackController.php

  /**
   * The current user.
   *
   * @var \Drupal\Core\Session\AccountInterface
   */
  protected $currentUser;

That property is already defined from the parent class.

  /**
   * The onlyoffice settings.
   *
   * @var \Drupal\Core\Config\Config
   */
  protected $moduleSettings;

There is no need to use that property, since the parent class has a property for the configuration factory.

  /**
   * A logger instance.
   *
   * @var \Psr\Log\LoggerInterface
   */
  protected $logger;

The parent class has already a method to get the logger service.

onlyoffice.module

  if ($entity->getEntityTypeId() != "media") {
    return NULL;
  }

hook_entity_operation() implementations return an array, not NULL. Since they are invoked with $operations += $this ->moduleHandler() ->invokeAll('entity_operation', [$entity]); NULL would cause issues.

There is no need to repeat the GPL notices for each file. Project hosted on drupal.org are under the same license used by Drupal core. The LICENSE file is also added from the packaging script.

apaderno → , thank you for your comments, we will fix this. I do not understand one point. To get the logger variable, I use the parent class method L154. I don't understand what is the downside of using this. I even found a similar usage in Drupal source code L71. Can you point out what is the disadvantage of such use?

The point is about defining a property (protected $logger;) already defined from the parent class. The class should use the existing property or the methods defined in the parent class.

Also, since this application has been created by onlyoffice, it is onlyoffice who needs to ask what a review meant.

Thank you for checking the code.
The use of the $currentUser property and the $moduleSettings property has been replaced.
Fixed returning an array in a onlyoffice.module.
Removed GPL notices in all files.

But the $logger property was not found in the parent (ControllerBase) class.
Calling the method once in the constructor looks more correct. Just like it is done in Drupal core.

The first lines for the ControllerBase class are the following one.

abstract class ControllerBase implements ContainerInjectionInterface {
  use LoggerChannelTrait;
  use MessengerTrait;
  use RedirectDestinationTrait;
  use StringTranslationTrait;

You will not find that property in the parent class, but that does not mean it is necessary, given the parent class is using those traits.

We made changes to get the logger service.

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