Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The ONLYOFFICE module enables users to edit files in the Media module from Drupal using ONLYOFFICE Docs packaged as Document Server. You will need an instance of ONLYOFFICE Docs (Document Server) that is resolvable and connectable both from Drupal and any end clients. ONLYOFFICE Document Server must also be able to POST to Drupal directly.
Project link:
Issue fork projectapplications-3341921
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Jitendra verma CreditAttribution: Jitendra verma as a volunteer and at TO THE NEW commentedThank 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.
Comment #3
jitesh_1Hello @onlyoffice,
Remove the develop branch , as branch names end with the literal .x. That branch needs to be removed.
Comment #4
jitesh_1Comment #5
vishal.kadamComment #6
vishal.kadamFILE: onlyoffice/src/OnlyofficeUrlHelper.php
\Drupal calls should be avoided in classes, use dependency injection.
Comment #7
apadernoThank you for applying!
Is the account used to create this application used by more than a person?
Comment #8
aleksandr.fedorov CreditAttribution: aleksandr.fedorov commentedHi 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?
Comment #9
apadernophpcs
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.
Comment #10
onlyoffice CreditAttribution: onlyoffice commentedThanks to all reviewers.
The develop branch has been removed.
The use of dependency injection is not required (by comment #9)
Comment #11
apadernoThere have not been answers to comment #7.
Comment #12
onlyoffice CreditAttribution: onlyoffice commentedOnly one person has access to account ONLYOFFICE
Comment #13
apadernoMay you then add your first name and last name to the account?
Comment #14
onlyoffice CreditAttribution: onlyoffice commentedNo problem. Added.
It is planned that this is a corporate account. But at the moment it is run by one person.
Comment #15
apadernoAn 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.
Each developer needs to create their own account, which must not be shared with other people, in the present nor the future.
Comment #16
onlyoffice CreditAttribution: onlyoffice commentedOk. 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.
Comment #17
apadernosrc/Controller/OnlyofficeCallbackController.php
That property is already defined from the parent class.
There is no need to use that property, since the parent class has a property for the configuration factory.
The parent class has already a method to get the logger service.
onlyoffice.module
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.
Comment #18
apadernoComment #19
aleksandr.fedorov CreditAttribution: aleksandr.fedorov commentedapaderno, 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?
Comment #20
apadernoThe 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.Comment #21
apadernoAlso, since this application has been created by onlyoffice, it is onlyoffice who needs to ask what a review meant.
Comment #22
onlyoffice CreditAttribution: onlyoffice commentedThank 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.
Comment #23
apadernoThe first lines for the
ControllerBase
class are the following one.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.
Comment #24
onlyoffice CreditAttribution: onlyoffice commentedWe made changes to get the logger service.
Comment #25
apadernoThank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:
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.
Comment #26
apaderno