Replace t() calls with $this->t() in classes

Hello Maintainers,
I have provided a patch to fix this issue.
To get more information visit https://www.drupal.org/project/drupal/issues/3115809 →
Please review it.

Thanks!

Thank you for submitting the patch.
I only recently took over maintainership of this module, and I absolutely agree this is the correct approach.
If you have time to convert the patch to a merge request, it would make it easier to review and verify the new GitLab CI tests.
Thanks for your help!

Okay sure,
Thanks for your review.

@bharath-kondeti,
Thanks for creating an MR.

MR !9 is working fine and can be moved RTBC +1.

Thank you both for your work on this. I have merged the change into the 3.x branch. I'd like to try to get a couple of other changes included before moving to the 3.11 release, but will get it into a release soon.

Oops, it looks like I was a bit premature in marking this fixed.

There is a problem that $this can't be used when class functions are called statically. This affects ScheduledPublish::propertyDefinitions. It causes the tests to fail. I'm looking at the best way to resolve. I'd prefer not to go back to t() calls.

Looking at other implementations of propertyDefinitions in core, they do seem to use t(), so setting back to that for now.

After struggling with a CI error reporting invalid YAML file, I tried rerunning the CI from another module, and it failed with the same error, so I think something is broken in the include file. Will try again tomorrow.

Reran CI this morning, and tests pass, though there are a lot of PHPCS issues. Will open a separate issue for PHPCS.

I have merged !10, which puts back t() calls from the static propertyDefinitions method. I'd welcome views on whether there is a better approach.

I'm leaving on "needs review" for now.

Okay,
@lostcarpark Thanks for your review.

Thanks for your contribution @Harshita mehna. I'll keep on "needs review" till I'm convinced this is the right approach for static members.

Looking some more at core examples, I have found two approaches.

First, using the t() function, such as in /web/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['value'] = DataDefinition::create('boolean')
      ->setLabel(t('Boolean value'))
      ->setRequired(TRUE);

    return $properties;
  }

And second, using new TranslatableMarkup(), as seen in /web/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DecimalItem.php:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['value'] = DataDefinition::create('string')
      ->setLabel(new TranslatableMarkup('Decimal value'))
      ->setRequired(TRUE);

    return $properties;
  }

I'm suspecting the latter approach is the more modern one, but would welcome any opinions before making a change.

I've opened MR !11 using the new TranslatableMarkup() approach. I have confirmed this passes the CI tests.

I'd appreciate if anyone could review before I merge.

Some manual testing indicates that the same problem occurs with the static validateElement in ScheduledPublishWidget.

Shall look at a separate issue to get this in the test coverage.

I was worried that the previous merged change had an error, so I've gone ahead and merged #11.

I would like to add the test case described in 📌 Add test cover for ScheduledPublishWidget Needs work before creating a release, as that would catch errors like this.

I'll leave this issue open another day or two, then mark fixed.

Afraid I found another instance of $this->t() in a static function, in the MultipleUpdatesForm class.

Changed to use new TranslatableMarkup().

I think this can be considered fixed now.

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