Problem/Motivation

Module maintainer asked me for a code review. Here's a patch. :)

Steps to reproduce

It's not really a bug, just some warnings from PHPCS.

Proposed resolution

Apply the patch.

Remaining tasks

Test for regressions.

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork charcount-3208475

Command icon 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

hotwebmatter created an issue. See original summary.

hotwebmatter’s picture

lhockley’s picture

Oh hey. Thanks for your contribution!

I am not too familiar with implementing patch updates. I believe that is sorta considered the "old way"?

Would it be possible for you to make these changes in an issue branch and open a merge request for my review?

hotwebmatter’s picture

I took a minute to make a Merge Request: https://git.drupalcode.org/project/charcount/-/merge_requests/1

You can ignore the patch I made in your issue queue.

lhockley’s picture

All the changes look reasonable. I just want to learn a little more about the changes to use ConfigFactoryInterface vs ConfigFactory. Again, really appreciate your contribution here.

hotwebmatter’s picture

From the Drupal API docs:

The class line either gives the default class that provides the service, or if the service uses a factory class, the interface for the service.

For comparison, see this service implementation of the DiffFormatter class in Drupal core:

  /**
   * Creates a DiffFormatter to render diffs in a table.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The config factory.
   */
  public function __construct(ConfigFactoryInterface $config_factory) {
    $config = $config_factory
      ->get('system.diff');
    $this->leading_context_lines = $config
      ->get('context.lines_leading');
    $this->trailing_context_lines = $config
      ->get('context.lines_trailing');
  }

Your $config_factory needs to implement the ConfigFactoryInterface because it is a factory class.

lhockley’s picture

Status: Active » Fixed

Thank you for the contribution, and the clarification on changes.

Status: Fixed » Closed (fixed)

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