The DbLogController class defines the following properties.

  /**
   * The module handler service.
   *
   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   */
  protected $moduleHandler;

  /**
   * The form builder service.
   *
   * @var \Drupal\Core\Form\FormBuilderInterface
   */
  protected $formBuilder;

Those properties have been already defined from the parent class, ControllerBase and they should be removed from the DbLogController class.

CommentFileSizeAuthor
#8 drupal-remove-duplicate-properties-3246471-8.patch1.55 KBavpaderno
#3 drupal-remove-duplicate-properties-3246471-3.patch839 bytesavpaderno

Comments

apaderno created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Removed from the DbLogController the properties already defined from the parent class » Remove from the DbLogController the properties already defined from the parent class
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new drupal-remove-duplicate-properties-3246471-3.patch839 bytes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: Remove from the DbLogController the properties already defined from the parent class » Remove from the DbLogController class the properties already defined from the parent class
dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Needs review » Needs work

@apaderno thanks for the patch. I think we should also change the users of $this->formBuilder and $this->moduleHandler to the more robust method provided by the parent class formBuilder() and moduleHandler()

vicheldt’s picture

vicheldt commented
Assigned: Unassigned » vicheldt

I'll find and change what @dagmar mentioned.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

The parent class only has getter methods, for example formBuilder().

protected function formBuilder() {
  if (!$this->formBuilder) {
    $this->formBuilder = $this
      ->container()
      ->get('form_builder');
  }
  return $this->formBuilder;
}

Since $this->formBuilder is already initialized from the class constructor, using that method doesn't make the code more robust.

I will provide a patch that uses $this->formBuilder() and $this->moduleHandler(), but I think it's the first patch that should be committed.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-remove-duplicate-properties-3246471-8.patch1.55 KB
vicheldt’s picture

vicheldt commented
Assigned: vicheldt » Unassigned

I just talked to someone about this and the first patch looked fine, if anyone else wants to check this to make sure it's fine, it'd be best.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the patches. I think both patches work as @vicheldt and @apaderno. I traced back on the change records when formBuilder and
moduleHandler() were introduced. And they were set to reduce boilerplate. So in theory we should try to reduce the code of the constructor and use the existing methods. I also counted how many instances of formBuilder() vs formBuilder-> are in core. And there is a clear winner of
using formBuilder()

$ rg formBuilder- | wc -l
41

$ rg formBuilder\(\) | wc -l
196

I will left the decision of what to commit to core maintainers.

@vicheldt thanks for your contribution as well!. you are welcome to take a look to other Novice issues that I tagged in the past.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: drupal-remove-duplicate-properties-3246471-8.patch, failed testing. View results

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

The error reported in the test result isn't caused by this patch. I am changing status back, and adding tests for Drupal 9.4.x.

Warning: Your XML configuration validates against a deprecated schema.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9629d09d405 to 10.0.x and 8417e5ff439 to 9.4.x. Thanks!

  • alexpott committed 9629d09 on 10.0.x 
    Issue #3246471 by apaderno, dagmar, vicheldt: Remove from the...

  • alexpott committed 8417e5f on 9.4.x 
    Issue #3246471 by apaderno, dagmar, vicheldt: Remove from the...

Status: Fixed » Closed (fixed)

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