In RecurlySubscriptionPlansForms the submitForm method uses \Drupal::configFactory(). The use of \Drupal in generally frowned upon from my understanding, so the config.factory service should be included with dependency injection. However, in this case it already is available to us through the FormBase class that we are extending.

So...

\Drupal::configFactory()->getEditable('recurly.settings')->set('recurly_subscription_plans', $recurly_subscription_plans)->save();

... becomes...

$this->config('recurly.settings')->set('recurly_subscription_plans', $recurly_subscription_plans)->save();

This might be in other spots throughout the codebase too, but hopefully this sheds some light on the issue so it can be fixed everywhere!

CommentFileSizeAuthor
#3 remove_use_of_global-2790181-3.patch20.29 KBmarkdorison
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

adamzimmermann created an issue. See original summary.

markdorison’s picture

Title: Remove use of global \Drupal object from forms » Remove use of global \Drupal::configFactory() call from forms
Version: 8.x-1.0-alpha3 » 8.x-1.x-dev

@adamzimmermann: Great feedback. Updated title to denote this issue scope is limited to the config factory calls. Other global calls can be addressed in other issues.

markdorison’s picture

Status: Active » Needs review
FileSize
20.29 KB
adamzimmermann’s picture

diff --git a/modules/recurlyjs/src/Form/RecurlyJsFormBase.php b/modules/recurlyjs/src/Form/RecurlyJsFormBase.php
...
@@ -17,7 +17,7 @@ abstract class RecurlyJsFormBase extends FormBase {

So this doesn't need to extend ConfigFormBase since you are only using get() and not set(), correct?

I didn't pull it down and run it, but it looks good to me! Nice work with cleaning it up everywhere.

markdorison’s picture

@adamzimmermann Exactly; thanks for the review.

walangitan’s picture

Status: Needs review » Reviewed & tested by the community

RTBC. Patch applied successfully and no issues found during testing.

  • markdorison committed c1b9a45 on 8.x-1.x
    Issue #2790181 by markdorison, adamzimmermann, walangitan: Remove use of...
markdorison’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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