Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
The Basic Auth test module contains usages of \Drupal:: static service calls in both controllers and tests. This does not follow Drupal's recommended dependency injection practices and reduces testability and maintainability.
Steps to reproduce
- Clone or access the Basic Auth module repository.
- Navigate to the directory containing the test classes.
- Search for instances of
\Drupal::within these classes. - Run the test suite and observe potential warnings or errors related to dependency injection.
Proposed resolution
Refactor the BasicAuthTestController to extend ControllerBase and replace static \Drupal:: service calls with dependency injection and helper methods:
- Replace
\Drupal::state()with$this->state() - Replace
\Drupal::service('page_cache_kill_switch')with constructor injection using the #[Autowire] attribute - Replace
\Drupal::config()in tests with$this->config()
These changes align the code with modern Drupal dependency injection practices, improve testability, and remove reliance on static service access.
Remaining tasks
- Alter the tests.
- Run the tests to confirm that no regressions or side effects have been introduced.
- Conduct a code review to ensure the change does not negatively affect test execution.
User interface changes
None
Introduced terminology
None
API changes
None
Data model changes
None
Release notes snippet
None
| Comment | File | Size | Author |
|---|
Issue fork drupal-2699565
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 11.x
changes, plain diff MR !11414 - 2699565-replace-drupal-with compare
Comments
Comment #2
Swetha Yarla commentedI have replaced \Drupal:: with $this->container->get() in test classes of Basic Auth module.
Here is the attached patch which fixes this issue.
Comment #3
heykarthikwithuupdated with a minor change.
Comment #4
dimaro commentedI'm not found more occurrences of \Drupal:: in this class.
Looks good!
Comment #6
dimaro commentedPlease, can anyone check if the latest patch really fix the issue?
Comment #7
alexpott@dimaro
That's kind of what you're supposed to do when you rtbc the issue :)
You've checked one thing in #4. However, the key point here is that there are still discussions on going in #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests as to whether this is even desirable!
To be honest I'd rather refocus this issue to do a better test. We should change the site name before doing the drupalGet() and then assert that the realm is the new value. Plus it looks as though a site name
I like "quotations"might actually break something since there is no escaping \Drupal\basic_auth\Authentication\Provider\BasicAuth::challengeException(). So perhaps we could just re-scope this issue into something that addresses this.Comment #19
alvar0hurtad0Comment #21
alvar0hurtad0Hello,
Since a lot of things changed in the tests during the last 8 years I think we can focus on the scope of the issue and create a follow up issue to check if the component follows the recommendations in https://www.drupal.org/project/drupal/issues/2066993
I've created a MR with the requirements in this issue.
Comment #22
smustgrave commentedThanks for reviving the issue.
For good practice we should have a full summary template be used, explaining why this is needed.
Standard issue template is fine and if sections don't apply you can leave blank or better leave NA
Comment #23
realityloop commentedCreating an issue summary is a great Novice task for Contribution workshop at Drupalcon
Comment #24
daniel_mm02 commentedComment #25
daniel_mm02 commentedHi everyone, I've updated the issue summary with the necessary details regarding the replacement of
\Drupal::with$this->container->get()in our test classes.Comment #26
sivaji_ganesh_jojodae commentedInside
core/modules/basic_auth/testsI can still find the usage of\Drupal::. Should we change the following occurances as well?Comment #27
alexpottFunctional tests should use \Drupal KernelTests should not. so the issue title is incorrect. Sure we can inject services correctly into the controller...
Comment #28
dalinI'm at DrupalCon Atlanta2025 and I tried to sit down with a novice, but it turned out to be a bit much. I'm going to remove the Novice tag because this requires first understanding unit tests, dependency injection, and the difference between kernel tests and functional tests. Even though in the end we'll only be changing a few characters.
Comment #31
alvar0hurtad0Working on it.
Comment #32
alvar0hurtad0Comment #33
smustgrave commentedBig auth appears to be failing.
New parameters should be typehinted also
If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #34
alvar0hurtad0Hi ,
I'd like to finish it, but I don't get your comment Smustgrave. I'm sorry if I'm doing anything incorrect.
If I'm should stop contributing here, please let me know. I just want to get more involved in code contributions, but I don't want to bother anyone.
Comment #35
smustgrave commentedYou’re completely good! And doesn’t bother anyone
If you click into the pipeline you’ll see a functional test for big auth is failing so the change may need to be tweaked
The functional JavaScript failure is most likely random and just needs to be re ran
Comment #36
alvar0hurtad0Thanks for the clarification, Stephen.
:)
Working on it.
Comment #37
alvar0hurtad0Still working on this, I didn't forget.
Comment #38
alvar0hurtad0The test related to this are green now.
There're some javascript test failing but I can't find any relation with the changes done in this fork.
[EDIT]
I've rerun the tests and are green now.
Comment #39
smustgrave commentedLeft 1 comment on MR.
Comment #40
alvar0hurtad0Thanks for your review and sorry for the ping pong.
Comment #41
smustgrave commentedThis appears to be addressed.
Comment #43
alexpottI think we should retitle this issue. It's not really about test classes anymore. Plus I left quite a bit of feedback on the MR.
Comment #45
sivaji_ganesh_jojodae commentedI’ve addressed the feedback from the MR, rebased the branch, and the pipeline is now passing successfully.
Comment #46
sivaji_ganesh_jojodae commentedComment #47
smustgrave commentedBut now the summary doesn't match.
Comment #48
sivaji_ganesh_jojodae commentedComment #49
sivaji_ganesh_jojodae commentedI've updated the summary.
Comment #50
smustgrave commented@sivaji this is fine but PLEASE stop using AI for issue summaries, if #3585894: LLM harm reduction in Drupal core contribution, AGENTS.md guidelines it'll be even more discouraged and if #3574093: Ban slop issue summaries and comments lands not accepted, that 2nd is further off. Much appreciated if considered
Comment #51
alexpottCommitted and pushed e6c69fa80d4 to main and cd1bc46ff10 to 11.x. Thanks!