Problem/Motivation

\Drupal\Core\Config\ConfigFactoryInterface has missing or generic variable types for @param and @return documentation.

Proposed resolution

Add types or improve existing ones.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#10 interdiff.txt364 bytesXano
#10 drupal_2191911_10.patch1.67 KBXano
#6 drupal_2191911_6.patch1.26 KBXano
#1 drupal_2191911_1.patch2.46 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Assigned: Xano » Unassigned
Status: Active » Needs review
FileSize
drupal_2191911_1.patch2.46 KB
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Thanks!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Just as little advertisment block: These changes are also part of #2184231: Use ConfigFactoryInterface to type hint ConfigFactory

+1

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Well we don't need both issues/patches then, and if I commit this one it will break the other patch, right? Which one do you want to use? Please either take this stuff out of the other patch or mark this one as a duplicate.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Postponed (maintainer needs more info) » Closed (duplicate)

I merged this patch with the one from #2184231: Use ConfigFactoryInterface to type hint ConfigFactory.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Closed (duplicate) » Needs review
FileSize
drupal_2191911_6.patch1.26 KB

Adding these changes to #2184231: Use ConfigFactoryInterface to type hint ConfigFactory was not appreciated, so here is a new patch that no longer touches the same code as the patch in the other issue.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

What about the rest of the patch in #1? Let's maybe wait a bit and see what eventually gets included in that other patch and what is rejected as out of scope?

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

#2184231-47: Use ConfigFactoryInterface to type hint ConfigFactory indicates that all docblock changes unrelated to @return $this were out of scope, so I left them in that issue and removed them from the patch here.

The patches no longer touch the same code and together fix all remaining docs issues.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Maybe after that one gets in, we can also fix this from that other patch then:

+++ b/core/lib/Drupal.php
@@ -266,7 +266,7 @@ public static function config($name) {
    * factory. For example, changing the language, or turning all overrides on
    * or off.
    *
-   * @return \Drupal\Core\Config\ConfigFactory
+   * @return \Drupal\Core\Config\ConfigFactoryInterface
    */
   public static function configFactory() {

That needs a description added to the @return. Probably out of scope on the other issue?

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
FileSize
drupal_2191911_10.patch1.67 KB
interdiff.txt364 bytes

What do you think of this?

The other issue has been fixed, by the way.

Status: Needs review » Needs work

The last submitted patch, 10: drupal_2191911_10.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

Yet another ImageFieldDisplayTest failure. Dont' bother with hitting retest, it's definitely not due to this documentation-only patch.

Anyway, this looks fine, thanks!

alexpott’s picture

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

Committed bac3a5d and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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