Problem/Motivation

As we have seen in #2781207: Use class resolver for abstracted Content Moderation classes the class_resolver

Proposed resolution

Let's add it to \Drupal

Advantages

  • Discoverability
  • Documentability

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#19 interdiff.txt696 bytesclaudiu.cristea
#19 2785355-19.patch1.68 KBclaudiu.cristea
#13 interdiff.txt654 bytesdawehner
#13 2785355-13.patch1.67 KBdawehner
#11 interdiff.txt551 bytestimmillwood
#11 2785355-11.patch1.53 KBtimmillwood
#4 interdiff.txt794 bytestimmillwood
#4 2785355-3.patch1.34 KBtimmillwood
#2 2785355-2.patch578 bytestimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Active » Needs review
FileSize
2785355-2.patch578 bytes

Initial implementation.

Did wonder if we should return the basic class resolver object, or accept a class name and pass directly to getInstanceFromDefinition()? Went for returning a basic class resolver object.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

+1 for not doing some additional magic beside getting the service from the container.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
FileSize
2785355-3.patch1.34 KB
interdiff.txt794 bytes

Adding test.

dawehner’s picture

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

WFM

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Wait, actually I think we should document legit usecases of this function

The last submitted patch, 2: 2785355-2.patch, failed testing.

The last submitted patch, 4: 2785355-3.patch, failed testing.

The last submitted patch, 4: 2785355-3.patch, failed testing.

The last submitted patch, 4: 2785355-3.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
2785355-11.patch1.53 KB
interdiff.txt551 bytes

Adding basic docs to the method.

The last submitted patch, 2: 2785355-2.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2785355-13.patch1.67 KB
interdiff.txt654 bytes

Small expansion of the documentation.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

WFM

As long as the test passes, RTBC!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented 
+++ b/core/lib/Drupal.php
@@ -300,6 +300,22 @@ public static function cache($bin = 'default') {
+   * an object of a class that implements \Drupal\Core\DependencyInjection\ContainerInjectionInterface.

Nitpick: I think the classname should be on the next line to respect 80 cols.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

@borisson_ IIRC the 80 cols restriction is only for the first line, but I might be wrong. I did think about putting the class name on the third line, but that then made the third line longer than the second, which looked odd.

Anyway, I'll leave it up to the committer to do on commit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Nope the 80 chars limit is for every line.

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below).

from https://www.drupal.org/node/1354

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2785355-13.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon for Nuvole commented
Status: Needs work » Needs review
FileSize
2785355-19.patch1.68 KB
interdiff.txt696 bytes

Fixing #15.

dawehner’s picture

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

Thank you @claudiu.cristea

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

I was hesitant about this issue based on the title/summary, but the documentation in the patch itself makes a compelling case for this API addition. Committed 9d3d930 and pushed to 8.3.x. Thanks!

  • xjm committed 9d3d930 on 8.3.x 
    Issue #2785355 by timmillwood, dawehner, claudiu.cristea, borisson_:...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
+++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
@@ -102,7 +102,7 @@ public function testDatabase() {
-   * Tests the service() method.
+   * Tests the cache() method.

Well this change was out of scope, but oops, committed anyway. :)

Status: Fixed » Closed (fixed)

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