Problem/Motivation

drupal_get_message() and drupal_set_message() replaced by Messenger service, see https://www.drupal.org/node/2774931

drupal_set_message() was partially replaced in #2968718: drupal_set_message function declaration in VerboseMessengerTest prevents the function from being flagged as @deprecated in PHPStorm. This issue will replace the remaining calls.

Proposed resolution

Replace existing calls to the drupal_set_message() function with calls to the Messenger service

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#32 interdiff_30_32.txt438 bytessubson
#32 remove_deprecated_drupal_message-2971259-32.patch8.34 KBsubson
#30 remove_deprecated_drupal_message-2971259-29.patch8.48 KBsubson
#28 remove_deprecated_drupal_message-2971259-28.patch8.48 KBsubson
#24 add_dependency_injection_remove_deprecated_drupal_message-2971259-24.patch65.79 KBalesbencina
#21 deprecated_drupal_set_message-2971259-21.patch18.1 KBvakulrai
#19 deprecated_drupal_set_messag-2971259-11.patch17.76 KBvakulrai
#17 deprecated_drupal_set_message-2971259-11.patch18.08 KBvakulrai
#15 deprecated_drupal_set_msg-2971259-11.patch17.75 KBvakulrai
#13 fix_translation_class-2971259-13.patch64.47 KBalesbencina
#12 fix_coding_standards-2971259-12.patch64.47 KBalesbencina
#11 fix_coding_standards-2971259-11.patch55.84 KBalesbencina
#7 replace_deprecated_drupal_set_message-12605520-7.patch12.48 KBalesbencina
#6 replace_deprecated_drupal_message-2971259-6.patch12.38 KBalesbencina
#5 replace_deprecated_drupal_message-2971259-5.patch23.88 KBalesbencina
#4 replace_deprecated_drupal_message-2971259-4.patch23.81 KBalesbencina
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr created an issue. See original summary.

nkoporec’s picture

Assigned: Unassigned » nkoporec

I'm on it.

alesbencina’s picture

Assigned: nkoporec » alesbencina

Reassigned.

alesbencina’s picture

Added patch to remove deprecated function.

alesbencina’s picture

Replaced Drupal core message service with pathauto.verbose_messanger.

alesbencina’s picture

alesbencina’s picture

nkoporec’s picture

Assigned: alesbencina » Unassigned
Status: Active » Reviewed & tested by the community

Tested the latest patch and it looks good. I think we can mark it RTBC.

idebr’s picture

Status: Reviewed & tested by the community » Needs work

@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

  1. +++ b/src/Form/PathautoAdminDelete.php
    @@ -20,13 +21,21 @@ class PathautoAdminDelete extends FormBase {
    +   * Drupal messenger service
    

    Doc comment short description must end in a full stop.

  2. +++ b/src/Form/PathautoAdminDelete.php
    @@ -20,13 +21,21 @@ class PathautoAdminDelete extends FormBase {
    -   *   The alias type manager.
    

    This parameter comment is required by the Drupal coding standards

  3. +++ b/src/Form/PathautoAdminDelete.php
    @@ -20,13 +21,21 @@ class PathautoAdminDelete extends FormBase {
    +   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
    

    Missing parameter comment

  4. +++ b/src/Form/PathautoAdminDelete.php
    @@ -20,13 +21,21 @@ class PathautoAdminDelete extends FormBase {
    +    $this->messengerService = $messenger;
    

    Let's call this property "messenger" in line with Drupal core

  5. +++ b/src/Form/PathautoAdminDelete.php
    @@ -133,14 +143,17 @@ class PathautoAdminDelete extends FormBase {
    +        $this->messengerService->addMessage($this->t('All of your %label path aliases have been deleted.', [
    +          '%label' => $alias_type->getLabel()
    +        ]));
    

    A comma should follow the last multiline array item

  6. +++ b/src/Form/PathautoAdminDelete.php
    @@ -168,17 +181,22 @@ class PathautoAdminDelete extends FormBase {
    +          \Drupal::service('messenger')->addMessage(t('All of your automatically generated %label path aliases have been deleted.',[
    +            '%label' => $label
    +          ]));
    

    A comma should follow the last multiline array item

  7. +++ b/src/Form/PathautoAdminDelete.php
    @@ -168,17 +181,22 @@ class PathautoAdminDelete extends FormBase {
    +      \Drupal::service('messenger')->addMessage(t('An error occurred while processing @operation with arguments : @args',[
    +        '@operation' => $error_operation[0],
    +        '@args' => print_r($error_operation[0])
    +      ]));
    

    A comma should follow the last multiline array item

  8. +++ b/src/Form/PathautoBulkUpdateForm.php
    @@ -149,15 +149,18 @@ class PathautoBulkUpdateForm extends FormBase {
    +      \Drupal::service('messenger')->addMessage(t('An error occurred while processing @operation with arguments : @args'),[
    +        '@operation' => $error_operation[0],
    +        '@args' => print_r($error_operation[0])
    +      ]);
    

    A comma should follow the last multiline array item.

  9. +++ b/src/Form/PatternDisableForm.php
    @@ -12,6 +14,31 @@ use Drupal\Core\Url;
    +   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
    

    Missing parameter comment.

  10. +++ b/src/Form/PatternDisableForm.php
    @@ -12,6 +14,31 @@ use Drupal\Core\Url;
    +  public function __construct(MessengerInterface $messenger) {
    +    $this->messengerService = $messenger;
    +  }
    

    Let's call this property "messenger" in line with Drupal core

  11. +++ b/src/Form/PatternDisableForm.php
    @@ -44,7 +71,9 @@ class PatternDisableForm extends EntityConfirmFormBase {
    +    $this->messengerService->addMessage(t('Disabled pattern %label.',[
    +      '%label' => $this->entity->label()
    +    ]));
    

    Use $this->t instead of t() when possible
    A comma should follow the last multiline array item.

  12. +++ b/src/Form/PatternEditForm.php
    @@ -43,6 +44,11 @@ class PatternEditForm extends EntityForm {
    +   * @var \Drupal\Core\Messenger\MessengerInterface
    +   */
    +  protected $messengerService;
    

    Missing short description in doc comment.

  13. +++ b/src/Form/PatternEditForm.php
    @@ -61,12 +68,14 @@ class PatternEditForm extends EntityForm {
    +   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
    

    Missing parameter comment

  14. +++ b/src/Form/PatternEditForm.php
    @@ -61,12 +68,14 @@ class PatternEditForm extends EntityForm {
    +    $this->messengerService = $messenger;
    

    Let's call this property "messenger" in line with Drupal core.

  15. +++ b/src/Form/PatternEnableForm.php
    @@ -12,6 +14,28 @@ use Drupal\Core\Url;
    +   * @var \Drupal\Core\Messenger\MessengerInterface
    

    Missing short description in doc comment

  16. +++ b/src/Form/PatternEnableForm.php
    @@ -12,6 +14,28 @@ use Drupal\Core\Url;
    +   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
    

    Missing parameter comment

  17. +++ b/src/Form/PatternEnableForm.php
    @@ -12,6 +14,28 @@ use Drupal\Core\Url;
    +  function __construct(MessengerInterface $messenger) {
    

    Visibility must be declared on method "__construct"

  18. +++ b/src/Form/PatternEnableForm.php
    @@ -12,6 +14,28 @@ use Drupal\Core\Url;
    +    $this->messengerService = $messenger;
    

    Let's call this property "messenger" in line with Drupal core.

  19. +++ b/src/Form/PatternEnableForm.php
    @@ -12,6 +14,28 @@ use Drupal\Core\Url;
    +  }
    

    Expected one blank line after function, found 0

  20. +++ b/src/PathautoGenerator.php
    @@ -335,7 +335,7 @@ class PathautoGenerator implements PathautoGeneratorInterface {
    +      \Drupal::service('messenger')->addWarning($e->getMessage());
    

    Use dependency injection in classes when possible.

  21. +++ b/src/Plugin/pathauto/AliasType/EntityAliasTypeBase.php
    @@ -274,7 +274,9 @@ class EntityAliasTypeBase extends ContextAwarePluginBase implements AliasTypeInt
    +      \Drupal::service('messenger')->addMessage(\Drupal::translation()->formatPlural(count($ids), 'Updated 1 %label URL alias.', 'Updated @count %label URL aliases.'),[
    +        '%label' => $this->getLabel()
    +      ]);
    

    - Use dependency injection in classes when possible
    - Expected one space after comma, found 0.
    - A comma should follow the last multiline array item.

alesbencina’s picture

Thank you for reviews. I will fix coding standards.

alesbencina’s picture

Fixed coding standards.

alesbencina’s picture

Fix coding standards..

alesbencina’s picture

Changed translation class (t to $this->t).

alesbencina’s picture

Status: Needs work » Needs review
vakulrai’s picture

Adding a patch to fix code errors and dependency injection.

Status: Needs review » Needs work

The last submitted patch, 15: deprecated_drupal_set_msg-2971259-11.patch, failed testing. View results

vakulrai’s picture

Status: Needs work » Needs review
FileSize
18.08 KB

Fixed lint errors.

Status: Needs review » Needs work

The last submitted patch, 17: deprecated_drupal_set_message-2971259-11.patch, failed testing. View results

vakulrai’s picture

Status: Needs work » Needs review
FileSize
17.76 KB

Status: Needs review » Needs work

The last submitted patch, 19: deprecated_drupal_set_messag-2971259-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vakulrai’s picture

Status: Needs work » Needs review
FileSize
18.1 KB

Status: Needs review » Needs work

The last submitted patch, 21: deprecated_drupal_set_message-2971259-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

nkoporec’s picture

Tested #13 patch and the major part of issue mentioned in #12 is fixed.I just found one minor issue:

      $this->messenger->addMessage(\Drupal::translation() 
        ->formatPlural(count($ids), 'Updated 1 %label URL alias.', 'Updated @count %label URL aliases.'), [ 
          '%label' => $this->getLabel(), 

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.

alesbencina’s picture

Injected string translation manager, where was possible.

@vakulrai please use module "Testing" and run tests locally.

alesbencina’s picture

Status: Needs work » Needs review
msankhala’s picture

@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:

  1. Create another issue to fix coding standard with the whole module. In this patch only fix the coding standard for those lines where you are modifying or adding.
  2. Use MessengerTrait instead 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/2774931
msankhala’s picture

Status: Needs review » Needs work
subson’s picture

Status: Needs work » Needs review
FileSize
8.48 KB

Creating a patch just to replace drupal_set_message() to use MessengerTrait wherever possible.

Status: Needs review » Needs work

The last submitted patch, 28: remove_deprecated_drupal_message-2971259-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

subson’s picture

Fixing failures, need to call messenger() function in PatternEnableForm.php

subson’s picture

Status: Needs work » Needs review
subson’s picture

msankhala’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm the patch #32 applies cleanly and remove all the drupal_set_message() with an appropriate injection of Messanger service wherever possible Or use Drupal::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 the drupal_set_message() call has been replaced. Moving this to RTBC

Berdir’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/src/Form/PatternDisableForm.php
@@ -5,12 +5,15 @@ namespace Drupal\pathauto\Form;
 class PatternDisableForm extends EntityConfirmFormBase {
 
+  use MessengerTrait;
+

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

  • Berdir committed 8ed5c08 on 8.x-1.x authored by subson
    Issue #2971259 by alesbencina, vakulrai, subson: Replace usages of the...

Status: Fixed » Closed (fixed)

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