Closed (fixed)
Project:
Graph Mail
Version:
1.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
13 Nov 2022 at 12:31 UTC
Updated:
19 Dec 2022 at 21:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
kuntal_d commentedInjected dependencies in GraphMail plugin.
Please review.
Comment #3
avpadernoI will make this issue broader, since phpcs reports also when a class doesn't inject its dependencies.
Comment #4
avpadernoComment #5
avpadernoA documentation comment for a method defined in an interface should not be empty.
When adding new parameter to a method, those parameters need to be added at the end. Also, the code should be backward compatible.
That class is used to implement a service.
Comment #6
chandreshgiri gauswami commentedI will work on this issue.
Comment #7
avpadernoComment #8
chandreshgiri gauswami commentedProviding patch with fix.
Comment #9
kuntal_d commentedApplied the patch #8. It applied cleanly and resolved all coding standard issues. Attaching screenshot for the reference.
Comment #10
avpadernoThank you for working on this!
Since the code uses only a single logger channel, there is no need to store the logger factory. Only the logger channel is necessary.
When a dependency is added, the code should be similar to the following one, used by Drupal core in
GDToolkit::__construct().(I am referring to what that code does with
$file_system, which is a parameter with a default value.)Since
StringTranslationTraitis used, the constructor needs to also callsetStringTranslation()passing the translation service, which will then need to be one of the constructor's parameters.There should not be any empty line before the closing curly parenthesis.
The first argument of
log()must be a literal string, not the output of$this->t()ort(). I would also use code similar to the code shown forwatchdog_exception(). (I won't callwatchdog_exception(), as that is going to be deprecated.)Comment #11
avpadernoComment #12
chandreshgiri gauswami commentedI will work on it.
Comment #13
chandreshgiri gauswami commentedAttaching new patch with specified changes.
Only below point is not done. I saw multiple references using the same method which I have implemented. Someone else may also provide new patch with this point.
Comment #14
avpadernoComment #15
avpadernoBefore fixing the issues reported by
phpcs, I will implement some other changes that will change or reduce the required dependencies.Comment #16
avpadernoComment #18
avpaderno