Closed (fixed)
Project:
Drupal core
Version:
10.1.x-dev
Component:
other
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
2 Feb 2023 at 10:05 UTC
Updated:
7 Mar 2023 at 09:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
royalpinto007Comment #4
royalpinto007Hi @quietone,
With this commit, I've refactored the ConfigController class by implementing the ContainerInjectionInterface and utilizing dependency injection to pass in its required dependencies. This change improves the code maintainability and flexibility.
Please let me know if you have any questions or concerns.
Thanks!
Comment #5
quietone commented@royalpinto007, Welcome to Drupal! Thanks for the description of the changes!
The change is failing the pre commit checks. You can run these locally before submitting a change, Run core development checks.
Comment #6
rassoni commentedFixed failing the pre commit checks.
Comment #8
quietone commented@Rashmisoni, thanks for working on this. It is preferred that we stay with the existing merge or patch workflow instead of switching. As a reviewer it can take a while to figure out which should be reviewed. Oh, It is not necessary to test a patch on every combination of php and database. Just take the default option. If you look at the failures you will see that it is a FunctionalJavascript test that is failing. Drupal has a number of those tests that fail randomly. You can learn more about that in #2829040: [meta] Known intermittent, random, and environment-specific test failures.
Setting back to Needs review,
Comment #9
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
The random failure in #6 seems to be unrelated.
Applied patch locally and verified instances of t() have been replaced with this->t()
The issue summary mentions 3 files and that's all that was covered so good there.
Think this is good.
Comment #10
catchCommitted/pushed to 10.1.x, thanks!
Comment #12
jonathan1055 commentedFixed spelling in title and corrected the sniff in issue summary.