Problem/Motivation

@var in docblock of configFactory property in Drupal\Core\Controller\ControllerBase class type-hinted to Drupal\Core\Config\Config but it holds Drupal\Core\Config\ConfigFactory

Proposed resolution

Remaining tasks

CommentFileSizeAuthor
#10 interdiff-4-10.txt429 bytespk188
#10 Docblock-of-configFactory-property-ControllerBase-2881884-10.patch1.08 KBpk188
#8 2881884-8-fixedwrongdocblock.patch446 byteshardikpandya
#4 Docblock-of-configFactory-property-ControllerBase-2881884-4.patch1.08 KBDevaraj johnson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85 created an issue. See original summary.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Issue tags: +Novice
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Issue summary: View changes
Devaraj johnson’s picture

Devaraj johnson CreditAttribution: Devaraj johnson as a volunteer and commented
FileSize
Docblock-of-configFactory-property-ControllerBase-2881884-4.patch1.08 KB

Updated the patch please have a look

Devaraj johnson’s picture

Devaraj johnson CreditAttribution: Devaraj johnson as a volunteer and commented
Status: Active » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -70,7 +70,7 @@
    *
-   * @var \Drupal\Core\Config\Config
+   * @var \Drupal\Core\Config\ConfigFactory
    */
   protected $configFactory;

We should typehint to the interface, see \Drupal\Core\Config\ConfigFactoryInterface

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Status: Needs review » Needs work
hardikpandya’s picture

hardikpandya CreditAttribution: hardikpandya as a volunteer commented
Status: Needs work » Needs review
FileSize
2881884-8-fixedwrongdocblock.patch446 bytes

Added patch.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Status: Needs review » Needs work

It doesn't look like your patch address #6 Its exactly same as #4

pk188’s picture

pk188 CreditAttribution: pk188 at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
Docblock-of-configFactory-property-ControllerBase-2881884-10.patch1.08 KB
interdiff-4-10.txt429 bytes

#8 was incorrect. I applied the changes suggested in #6.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thank you

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented

+1 to RTBC.

  • Gábor Hojtsy committed 3ec98ca on 8.3.x 
    Issue #2881884 by pk188, Devaraj johnson, vijaycs85, dawehner: Docblock...

  • Gábor Hojtsy committed 1421285 on 8.4.x 
    Issue #2881884 by pk188, Devaraj johnson, vijaycs85, dawehner: Docblock...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks all, good find, thanks for the fix.

Status: Fixed » Closed (fixed)

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

rajeevku’s picture

rajeevku CreditAttribution: rajeevku commented

this base class but use ContainerInjectionInterface instead, or even better be refactored to be trivial glue code

. This doesn't look grammatically correct. Should be changed to

Controllers that contain sufficiently complex logic should use ContainerInjectionInterface instead of the base class.

Should i open a follow up or this one will be re opened. Looking for everyone's opinion. Thank you.