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.
This could be quite a big patch and need a few re-rolls so the sooner we can get this in the better.
Initial patch was committed in #37, but didn't catch all instances.
Meta Issue.
#1916134: Remove module_* deprecated functions
Comment | File | Size | Author |
---|---|---|---|
#45 | drupal-2045931-replace-module-exists-calls-45.patch | 18.62 KB | Sutharsan |
#45 | interdiff-2045931-40-45.txt | 1.74 KB | Sutharsan |
Comments
Comment #1
benjy CreditAttribution: benjy commentedTagging
Comment #2
Sam152 CreditAttribution: Sam152 commentedI can take a look at this over the weekend. Keen to start looking into some core development.
Comment #3
Sam152 CreditAttribution: Sam152 commentedI have run through all the instances of module_exists and replaced with the updated method. I have also removed the deprecated function as per Crells recommendation. It didn't seem to break anything, hopefully it can pass the testbot so it doesn't have to be rerolled too many times.
Comment #4
benjy CreditAttribution: benjy commentedNot sure if we are removing the functions at this point. @catch mention in IRC we should leave them.
Updated status for test bot.
Comment #5
Sam152 CreditAttribution: Sam152 commentedHere is a version without the deprecated function removed. Either one should work depending on the final decision made in this issue.
Comment #6
Sam152 CreditAttribution: Sam152 commentedComment #7
Sam152 CreditAttribution: Sam152 commentedI noticed a few places where \Drupal had not been 'use'd, so I will wait for the test results to come back negative and then fix up the individual cases when they arise.
I need to setup the test suite locally.
Comment #9
Sam152 CreditAttribution: Sam152 commentedThis patch makes appropriate use of \Drupal where required and includes the deprecated function as per tim.plunkett's suggestion.
Comment #10
Sam152 CreditAttribution: Sam152 commentedComment #11
benjy CreditAttribution: benjy commentedsee https://drupal.org/node/1353118 for when to use absolute vs relative. It seems if the class is global and the file has a namespace it should be absolute. However if the file does not have a namespace it should be relative.
Comment #12
Sam152 CreditAttribution: Sam152 commentedThanks for link. I believe the patch conforms to the standard after reading the docs.
Comment #13
benjy CreditAttribution: benjy commentedThis patch no longer applies. Any chance of a re-roll.
Comment #14
Sam152 CreditAttribution: Sam152 commentedYeah, I will manually repeat the work required for this patch as I doubt re-rolling would be very clean either. Will repost and hit up someone on IRC to have it applied quickly after it's creation.
Comment #15
Sam152 CreditAttribution: Sam152 commentedI have attached another patch. Would be good to get this committed as soon as it passes testing to prevent further duplication of work.
Comment #16
Sam152 CreditAttribution: Sam152 commentedComment #17
benjy CreditAttribution: benjy commentedLooks good, let's try get this in before it needs another re-roll.
Comment #18
alexpottThe current patch replaces module_exists() with \Drupal::moduleHandler.
I suggest that this patch broken into two. One part to deal with the procedural code (everything not in a /lib/... directory. And another to do the OO code correctly.
All the changes to procedural code in this patch look great!
What follows is a review of the OO code.
This is just one example from the patch... there are so many more I did not bother to list them all. In tests the simpletest runner has access to the container and therefore we can use
$this->container->get('module_handler')
instead of\Drupal::moduleHandler()
.These can be injected by implementing EntityControllerInterface and adding a createInstance method. For an example have a look at \Drupal\action\ActionAddFormController
This should be injected by updating serializer.normalizer.entity.hal definition in hal.services.yml perhaps using setter injection.
This can be injected with by creating a static create method on the class... see Drupal\views\Plugin\views\style\Table
Comment #19
Sam152 CreditAttribution: Sam152 commentedHi Alex,
Thanks for the review. I will take a look.
Sam
Comment #20
benjy CreditAttribution: benjy commentedHow we going with this?
Comment #21
Sam152 CreditAttribution: Sam152 commentedI have attached the patch for the procedural code only. The OO code can be tackled in a later patch.
Comment #22
benjy CreditAttribution: benjy commentedLooks good.
Comment #23
webchickno longer applies.
Comment #24
adsw12 CreditAttribution: adsw12 commentedI tried to re-rolling.
Comment #25
johnmcc CreditAttribution: johnmcc commentedThe patch in #24 applied cleanly for me and everything looks OK. (Although I had to delete sites/default/files/php to get up and running again.)
Comment #26
galooph CreditAttribution: galooph commentedAs per #25 - patch applied cleanly, but had to empty sites/default/files/php/ to get back up and running.
Comment #27
benjy CreditAttribution: benjy commentedWe've now standardised on "\Drupal" in core regardless of whether it's in a procedural or OO file so this patch needs updating to match that.
Comment #28
joelpittetRe-roll as the comment module was failing to apply. And fix from #27
Comment #29
johnmcc CreditAttribution: johnmcc commented#28 looks good and applies cleanly.
Comment #30
galooph CreditAttribution: galooph commented#28 applied cleanly and looks fine.
Comment #31
benjy CreditAttribution: benjy commentedLooks good.
Comment #32
LinL CreditAttribution: LinL commentedReroll of #28 (the 20.78kb one) as the first hunk in node.views.inc has been removed already in #2057401: Make the node entity database schema sensible
Otherwise it is the same.
Comment #34
LinL CreditAttribution: LinL commented#32: 2045931-32.patch queued for re-testing.
Comment #35
LinL CreditAttribution: LinL commentedAnd another reroll as
comment.module
andcomment.views.inc
are already done in #2003498: Replace drupal_container() with Drupal::service() in the comment moduleComment #36
benjy CreditAttribution: benjy commentedBack to RTBC
Comment #37
catchCommitted/pushed to 8.x, thanks!
Comment #39
ianthomas_ukReopening as there are still loads of calls to module_exists().
Comment #40
joelpittetOk here is all but the function itself. Should that be included as well or marked as deprecated?
Comment #41
ianthomas_ukThis is in OO code, so shouldn't use \Drupal. There are several examples of this in the patch.
Let's leave it for now, and then remove all the module_* functions together for the meta. It would be good to expand the comment though, to make it clear that it's not going to be around much longer. Suggested comment as discussed on #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed would be
Comment #42
ianthomas_ukComment #43
ianthomas_ukComment #44
ianthomas_ukRegarding using \Drupal in OO code, the same problem applies in #2055371: Replace all module_invoke_all() deprecated function calls in OO code, where it has been decided to use \Drupal for now, then do propery dependency injection in a later patch. We can do the same here DI is going to be tricky, but at the very least CustomBlockTypeFormController has a $moduleHandler property that we can use (I've not checked the others).
Comment #45
Sutharsan CreditAttribution: Sutharsan commentedReplaced two \Drupal::moduleHandler() calls in CustomBlockTypeFormController and ConfigTestFormController as mentioned in #44.
Comment #46
herom CreditAttribution: herom commentedmoving back to needs work, based on @ianthomas_uk's suggestion in #41 to either remove the deprecated function, or expand the comments.
Comment #47
ianthomas_ukActually, the functions for this meta are all next to each other, so if we remove or edit the docblock on one then it will trigger a reroll for one of the other patches.
Let's just remove the uses, then we can follow up with a quick patch to remove the functions themselves.
We still need a followup to switch to DI.
Comment #48
catchYes removing uses then separate patch to remove the actual functions is the easiest way to do these - reduces the chance of broken HEAD due to commit conflicts. And the removal patches themselves are much easier to re-roll/roll-back in case of conflicts.