Closed (fixed)
Project:
Pathauto
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
8 May 2018 at 07:29 UTC
Updated:
22 Sep 2018 at 11:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
nkoporecI'm on it.
Comment #3
alesbencina commentedReassigned.
Comment #4
alesbencina commentedAdded patch to remove deprecated function.
Comment #5
alesbencina commentedReplaced Drupal core message service with pathauto.verbose_messanger.
Comment #6
alesbencina commentedFixed failing tests.
Comment #7
alesbencina commentedCorrected code so assertText does not fail when using Messenger service.
Comment #8
nkoporecTested the latest patch and it looks good. I think we can mark it RTBC.
Comment #9
idebr commented@nkoporec / alesbencina Thanks for working on this issue. I recommend installing in your code editor, so you get notified when code violates the Drupal coding standards. You can find instructions at https://www.drupal.org/node/1419988
Doc comment short description must end in a full stop.
This parameter comment is required by the Drupal coding standards
Missing parameter comment
Let's call this property "messenger" in line with Drupal core
A comma should follow the last multiline array item
A comma should follow the last multiline array item
A comma should follow the last multiline array item
A comma should follow the last multiline array item.
Missing parameter comment.
Let's call this property "messenger" in line with Drupal core
Use $this->t instead of t() when possible
A comma should follow the last multiline array item.
Missing short description in doc comment.
Missing parameter comment
Let's call this property "messenger" in line with Drupal core.
Missing short description in doc comment
Missing parameter comment
Visibility must be declared on method "__construct"
Let's call this property "messenger" in line with Drupal core.
Expected one blank line after function, found 0
Use dependency injection in classes when possible.
- Use dependency injection in classes when possible
- Expected one space after comma, found 0.
- A comma should follow the last multiline array item.
Comment #10
alesbencina commentedThank you for reviews. I will fix coding standards.
Comment #11
alesbencina commentedFixed coding standards.
Comment #12
alesbencina commentedFix coding standards..
Comment #13
alesbencina commentedChanged translation class (t to $this->t).
Comment #14
alesbencina commentedComment #15
vakulrai commentedAdding a patch to fix code errors and dependency injection.
Comment #17
vakulrai commentedFixed lint errors.
Comment #19
vakulrai commentedComment #21
vakulrai commentedComment #23
nkoporecTested #13 patch and the major part of issue mentioned in #12 is fixed.I just found one minor issue:
I think we should inject service \Drupal::translation() too so we can fix one more issue along the way.
@vakulrai I don't understand why are you posting new patches? Did you test and review #13? Also before posting a patch, it's better to run tests locally to check if they pass.
Comment #24
alesbencina commentedInjected string translation manager, where was possible.
@vakulrai please use module "Testing" and run tests locally.
Comment #25
alesbencina commentedComment #26
msankhala commented@alesbencina Thanks for your hard work on this issue and continues effort of improving the patch. This patch is doing more than the scope of this issue like fixing the coding standard of the code which is not under the scope of this issue and moving the method within the class.
This is called scope creep. The scope of this issue is just to replace drupal_set_message with Messagener service. I'll suggest two things:
MessengerTraitinstead of injecting Messenger service inside constructor. Inside the constructor, you can call$this->setMessenger()and pass it messenger service. This will reduce the amount of code you need to write. See https://www.drupal.org/node/2774931Comment #27
msankhala commentedComment #28
subson commentedCreating a patch just to replace drupal_set_message() to use MessengerTrait wherever possible.
Comment #30
subson commentedFixing failures, need to call messenger() function in PatternEnableForm.php
Comment #31
subson commentedComment #32
subson commentedremoving extra line.
Comment #33
msankhala commentedI can confirm the patch #32 applies cleanly and remove all the
drupal_set_message()with an appropriate injection of Messanger service wherever possible Or useDrupal::service('messanger')in static method.grep -rn 'drupal_set_message' .running this command from inside this module directory after applying this patch return no result that means all thedrupal_set_message()call has been replaced. Moving this to RTBCComment #34
berdirThis should not be necessary as the parent already has the trait (it also extends from FormBase in the end).
Adding this could cause problems in some PHP versions.
Fixed on commit.