[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 , as branch names end with the literal .x. That branch needs to be removed.

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

That said, a static method cannot use Dependency Injection.
It is yet to see if those static methods need to be static. When I see a static class that still needs to use services, I always ask myself why a service is not instead used. I guess that is the same thing vishal.kadam thought.

There have not been answers to comment #7.

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

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.

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

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.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the reviewers.