Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up of #1937600: Determine what services to register in the new Drupal class
Replace all calls to config() with Drupal::config() and then remove the config() function.
Comment | File | Size | Author |
---|---|---|---|
#62 | followup-1957142-62.patch | 3.91 KB | jibran |
#62 | interdiff.txt | 702 bytes | jibran |
#58 | followup-1957142-58.patch | 4.25 KB | dawehner |
#52 | 1957142-52.patch | 432.07 KB | damiankloip |
#51 | 1957142-51.patch | 432.84 KB | damiankloip |
Comments
Comment #1
BerdirInstructions:
Search & replace all calls to config() with the mentioned method. Note that calls within namespaced classes (have a namespace line and are in the lib/ folder) needs to be "\Drupal".
Comment #2
xjmThis is liable to break every patch in core, so let's split it into smaller patches. Convert a module at a time or such.
Comment #3
xjmPostponing on the main issue.
Comment #4
BerdirI'm not convinced that splitting it up in this case helps. Splitting up helps when you actually need to do more than a simple search & replace. A nice example was t() in assertion messages.
Many config() conversions span across multiple modules, splitting this up would just conflict with them multiple times. I think we should just start to switch all ongoing patches to Drupal::config() and then define a date to roll and commit this.
Comment #5
xjmThe problem is that there is never truly a simple search-and-replace, and the patch is still impossible to review regardless of how fast you make it. If you split this up, you can let novices handle it gradually, and let other patches be rerolled gradually. Each issue has a slight overhead, but the overall process is much less painful.
Comment #7
damiankloip CreditAttribution: damiankloip commentedI think I agree with berdir here, I would rather see one patch for each method conversion, even though I managed to mess up the state() one :) That is mainly down to a lack of time and laziness. Yes, it is a large patch, but given a little time, it's relatively easy to go through.
If we split this up per method per module, we will end up with an issue count in the hundreds, which would take a monumental amount of time to get committed. It would be some sort of reroll hell for other patches; having to keep track of multiple issues doing this conversion, potentially.
Comment #8
xjmOK, but I'm not reviewing it. :)
Comment #9
webchickAs a committer, I also prefer this "one patch per method" approach. And so I guess I'll sign up to review it. :)
Comment #10
xjmComment #11
alexpottI think we should mark the config function deprecated so that we can do this after feature freeze #2028149: The config() function should be deprecated in favour of Drupal::config()
Comment #12
damiankloip CreditAttribution: damiankloip commentedThat seems sensible to me, although I still think we should reconsider making this just one patch. After api freeze this will become easier anyway.
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedFun!
Comment #15
damiankloip CreditAttribution: damiankloip commentedI think the replacement went a bit OTT there
Also, there were a couple of missing parts etc... :)
Comment #16
damiankloip CreditAttribution: damiankloip commentedAnd I guess this is not a meta issue anymore.
Comment #18
damiankloip CreditAttribution: damiankloip commentedOh, missed some stuff in ConfigImporter that needed changing from the original patch.
Comment #20
damiankloip CreditAttribution: damiankloip commentedSorry, unresolved merge conflict from rebasing.
Comment #22
fubhy CreditAttribution: fubhy commentedWe should replace Drupal::config // config() with $this->container->get('config.factory') in tests. And there are still a lot of places where we use Drupal::config() // config() in classes even though we could inject it properly.
Let's get it right in one go now that we are producing such a big patch anyways.
Comment #23
damiankloip CreditAttribution: damiankloip commentedChanging all the places to inject instead is going to make an absolutely massive patch!
Edit: if that is the patch...not too bad.
Comment #24
fubhy CreditAttribution: fubhy commentedNo, that patch does not do the injection, it just changes the test usage of the config factory to go with $this->container->get('config.factory');
Edit: Oh, I forgot *TestBase.php :)
Comment #25
damiankloip CreditAttribution: damiankloip commentedIn that case I would say going with injection is not a good plan for this patch.
Comment #26
fubhy CreditAttribution: fubhy commentedI think this should be it. There was just a very tiny amount of things can get the config factory injected. I will cover the ModuleHandler later as part of the disabled modules post-patch cleanup :).
Comment #27
fubhy CreditAttribution: fubhy commented@damiankloip even if it's just 5kb of the entire patch? (Which is 456kb large ;) ).
Comment #28
damiankloip CreditAttribution: damiankloip commentedOK, fair enough. I don't mind...I'm not reviewing it :-)
Comment #30
damiankloip CreditAttribution: damiankloip commentedI think the config query factory is just not passing in the new config factory param.
Comment #32
damiankloip CreditAttribution: damiankloip commentedThere is no $this->container at that stage when run() is called.
Comment #34
fubhy CreditAttribution: fubhy commentedNext try...
Comment #35
fubhy CreditAttribution: fubhy commentedInjecting in some more places and using $this->container->get('config.factory') in some base tests that we missed before.
Note: Per discussion with @damiankloip we are completely ignoring views plugins (or rather, any plugins) for now when it comes to injecting the config factory. Otherwise this patch would explode... Also, there will be dedicated cleanup patches for all those Views plugins anyways... We can solve that there.
This should be it now from my side. I just walked through all the changes once again and think we covered it all for now.
Comment #36
fubhy CreditAttribution: fubhy commentedMore fixes...
Comment #38
fubhy CreditAttribution: fubhy commentedShould be green now I guess.
Comment #39
damiankloip CreditAttribution: damiankloip commentedReroll.
Comment #40
dawehnerI really really really like the idea to split up the injection part from the procedural code, as done afaik on the module handler. This just makes the life easy for other people.
Out of scope!
> 80 chars
I personally think that we should store the config object instead of the full factory.
This is out of scope, sorry.
So why don't we inject it in here?
> 80 chars
> 80 chars.
+1
> 80 chars.
> 80 chars.
Why is there no injection applied? +1 to not do it in any example.
This is not injected, so it simply does not work at all!
> 80 chars
> 80 chars.
> 80 chars.
I think we should also document the mock object, to not show that we have used an invalid method call.
Wow, it is odd to use a class in a setting. Shouldn't that just be a service?
Out of scope.
We could also fix the double-quoting ;)
Comment #42
damiankloip CreditAttribution: damiankloip commentedYeah, I think I'm starting to agree. All the replacements - fine. Switching tests to use $this->container directly and injection, is a step too far imo.
Comment #43
damiankloip CreditAttribution: damiankloip commentedShall we just go back to this replacement then?
Comment #44
dawehnerInventing a new tag ;)
Comment #45
damiankloip CreditAttribution: damiankloip commentedI'm not retrospectively refilling the old patch (which is a pain) to get an interdiff! :-) this is basically a reroll of #21.
Comment #46
dawehnerMaybe we should better keep the api.
Comment #47
damiankloip CreditAttribution: damiankloip commentedYeah, you're right.
Comment #49
damiankloip CreditAttribution: damiankloip commentedSorry, forgot to rebase 8.x.
Comment #50
dawehnersorry.
Comment #51
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #52
damiankloip CreditAttribution: damiankloip commentedJust another reroll.
Comment #53
dawehnerThis looked fine already back in #46 (beside the config() removable) so a fast scan now haven't shown anything bad.
Comment #54
webchickAll right, awesome. As promised in #9, I read through this entire patch. I didn't spot any false-positives (that doesn't mean there aren't any :), just I didn't notice and was looking for them) and the regexes used even do fancy things like switch Drupal::config() for \Drupal::config() depending on whether or not it's in a class file. (That reminds me that I still really want #2053489: Standardize on \Drupal throughout core to happen, because the code looks wildly inconsistent right now to people who don't know that that's the trick.)
There's nothing currently tagged as "Avoid commit conflicts" so talked this situation over with catch, and he approved just getting this done and letting the chips fall where they may. Hopefully since this function was already deprecated, it won't break too much RTBC stuff in the queue.
So...
Committed and pushed to 8.x. Thanks!
Because we're after API freeze, let's get a small change notice out about this to notify module developers.
Comment #55
webchickAlso I went through quickly and updated all the change notices I could find with "config(" in them to do Drupal::config() instead.
Comment #56
dawehnerHere is one https://drupal.org/node/2065313
Comment #57
webchickChange notice looks great! I touched the language up slightly.
However, we missed a few spots. :)
Comment #58
dawehnerLet's fix them.
Comment #59
damiankloip CreditAttribution: damiankloip commentedSorry, must have got lost/missed as the patch got rerolled!
Comment #60
jibranThis is irreverent.
Comment #61
dawehnerYou are totally right!
Comment #62
jibranHere we go. I think #57 is addressed so RTBC. It is just a comment change so I went ahead and RTBC it.
Comment #63
webchickCommitted and pushed that to 8.x too. Thanks!
I think we can close this out now.
Comment #64
andypostThis needs follow-up a-la #2001206: Replace drupal_container() with Drupal::service() to replace \Drupal with $this->container
Comment #65
damiankloip CreditAttribution: damiankloip commentedYes, that for tests. And we need another follow up to inject config objects where we can instead of calling Drupal::config().
Comment #66
andypostFiled #2066993: Use \Drupal consistently in tests
And still needs another one to address #65