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.
All this functions are deprecated in #2109287: Replace list_themes() with a service.
So let's clean-up their usage
Comment | File | Size | Author |
---|---|---|---|
#88 | interdiff.txt | 510 bytes | JeroenT |
#88 | clean_up_usage_of-2151469-88.patch | 25.95 KB | JeroenT |
#85 | 2151469-85.patch | 25.96 KB | rpayanm |
Comments
Comment #1
Richard Damon CreditAttribution: Richard Damon commentedWorking on this
Comment #2
eporama CreditAttribution: eporama commentedJust to make sure, we're on the right track, we're talking about a *lot* of this type of change:
Should we have one massive patch for this? Or handle modules individually? And as Gábor mentions, individual modules may have a lot more things that need to be changed like config_translation which has code just to get around this.
Comment #3
Richard Damon CreditAttribution: Richard Damon commentedHere is a patch replacing all the depreciated procedural calls with calls to the service. Someone with a bit more understanding of things can look at what can be simplified because of this.
Comment #4
Richard Damon CreditAttribution: Richard Damon commentedComment #5
tstoecklerWow, quite a massive patch, nice work!
There's a detailed review below, which basically boils down to a few things:
If this is overwhelming (which I could definitely understand!!!) you could also avoid all the injection stuff for now and limit this issue to the changes in the test files and then let the rest be handled in a separate issue. That's your call. :-)
Follow-up issues that should be created:
This does not wrap at 80 characters, but more importantly this should say "ThemeHandlerInterface::listInfo()" instead of \Drupal::service...
Instead of this change, the theme handler should be injected (see __construct() in this class and the core.services.yml file)
and any usage of listThemes() in this class should be replaced by $this->themeHandler->listInfo() directly.
The theme handler should be injected instead (see the __construct() and create() methods in this class).
The theme handler should be injected here. (__contstruct() and create()).
BlockController doesn't use ContainerInjectionInterface yet, so I think it's fine to leave this as is.
For extra credit, though, you could inject this as well. (Look at how CustomBlockController does it for guidance.)
This should be injected (__construct() and create()).
Per the todo, this function should be removed, and instead a) the theme handler should be injected (see the config_translation.services.yml) and all usages of getThemeList() should be replaced with direct calls to the theme handler. Note: ConfigMapperManagerTest will have to be updated for the constructor change.
Weird wrapping here.
More importantly, this should be injected as well. Here you have to actually use createInstance() (instead of the usual create()). You can look at how EntityListController does this.
Here the theme handler should be injected (__construct() / create()).
Also, the system_list_reset() should not be necessary, $this->themeHandler->reset() already takes care of that.
This should be injected with the usual __construct() / create().
Since this is a plugin (derivative) I don't think we can get by without injecting the theme handler. Look at other local task derivatives on how they do that, e.g. FieldUiLocalTask.
We could avoid getting the service twice by doing $theme_handler = \Drupal::service('theme_handler') once and then using the local variable.
This does not wrap properly. Also, instead of just saying listInfo(), we should probably be specific and say ThemeHandlerInterface::listInfo().
Again, this doesn't wrap at 80 characters and should say ThemeHandlerInterface::listInfo() instead of \Drupal::....
We could store $theme_handler in a local variable here to avoid calling \Drupal::service() twice.
Again, this should say ThemeHandlerInterface::enable() and wrap at 80 characters.
This should say ThemeHandlerInterface::enable() and wrap at 80 characters.
Same as above, but here we need the full namespace: \Drupal\Core\Extension\ThemeHandlerInterface::enable()
This should *not* be injected as there's currently a lot of uninjected stuff in DisplayPluginBase and that should be a separate issue. In theory we could store $theme_handler in a local variable here, but that will make it slightly harder when actually injecting stuff. So, I don't really care, but I would leave it as is.
Here I personally find injection less important, but for extra credit you could use ContainerInjectionInterface as explained above.
Here we can store the theme handler in a local variable to save some cycles.
In theory we should really make enable() and disable() chainable but that's a different issue.
Comment #6
tstoecklerAlso Re #2: Let's open a follow-up issue to create a listEnabledInfo() (or similar) method on the theme handler that would encapsulate that code.
Comment #8
Richard Damon CreditAttribution: Richard Damon commentedI will look at the comments and update the patch. This was my project at today's code sprint in Boston. I will need a bit of study/instruction on the "injection" comments.
Comment #9
Richard Damon CreditAttribution: Richard Damon commentedOne question, would it be better to split the patch into a patch per core module being updated, so perhaps some can be updated quicker, or should I keep it all as one big patch for the full issue?
Comment #10
Richard Damon CreditAttribution: Richard Damon commentedHere is a patch with much of the fixes done. I haven't actually implemented full dependency injection into the classes, but have added the class member to hold the handler, and am initilizing it in the constructor. This sets things up for making the changes to implement the injection.
There is also one test that failed last time, apparently due to the change breaking an ad-hoc mocking of list_themes, which will need to be changed into a real mock.
I am tempted to break this into two patches, one for what is basically completed now, and that which still needs work, so that the issue has a big bite taken out of it for now.
Comment #12
lokapujyaThe last patch passed Simple test for me locally, but now this needs a re-roll.
Comment #13
Richard Damon CreditAttribution: Richard Damon commentedI'll look into re-rolling the patch this week.
Comment #14
pflame CreditAttribution: pflame commentedI will try to reroll the patch since it's been there for few weeks. I am working on this through core mentoring hours.
Comment #15
pflame CreditAttribution: pflame commentedI reroll the patch and uploaded the latest patch.
Comment #17
pflame CreditAttribution: pflame commentedI reroll the patch again, the patch in #15 has some test functions which are removed from the latest code. Please review the latest patch attached to this comment.
Comment #19
star-szr17: deprecated-2151469-17.patch queued for re-testing.
Comment #21
pflame CreditAttribution: pflame commented17: deprecated-2151469-17.patch queued for re-testing.
Comment #23
sunComment #24
lokapujyaWIP.
Comment #26
lokapujyaAm hoping for at least a test run.
Comment #28
lokapujyaanother quick try.
Comment #30
lokapujyaI don't know why the test run failed. None of the actual tests failed. We just had a random error. I'm trying just the changes on the block module.
Comment #32
lokapujyaDoes anyone knows why all tests pass, but I still get this error in Simpletest: PHP Fatal error: Call to a member function get() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal.php on line 138.
Next, Ill try a patch for a different module, not block.
Comment #33
lokapujyaThis includes the changes for only some of the modules, just to see if it passes simpletest.
Comment #35
lokapujyajust the color module changes?
Comment #36
lokapujyaRedo of #33. That patch had unintended changes in it. If this passes, the problem is most likely in the block module code changes.
Comment #37
ianthomas_ukThis doesn't currently have a patch to review.
I think the basics of this could be handled by #2208607: Write script to automate replacement of deprecated functions where possible, and then a followup patch here for documentation and dependency injection.
Comment #38
lokapujyaSummary of the last few patches: I couldn't figure out which change is causing the test to break. A patch with the changes that contains just the block modules changes failed Simpletest. Another patch with the changes for 6 other modules passed.
So, I still don't know why the changes in block module are not passing. Here is another test to see if it has anything to do with the protected themehandler variable.
Comment #39
lokapujyaComment #40
lokapujyaprotected themehandler was misued. Removal of that code passeed in #38.
Here the complete reroll: patch from #28 + removal of the protected $themehandler local variable (as passed in patch from #38).
Comment #42
lokapujyaSo this is a patch with all of the $themehandler local variables removed.
Comment #43
ianthomas_ukWe've got a better patch, so removing the link to the automated script issue.
Comment #44
lokapujyaRecommend implementing dependency injection in a followup: #2221801: Use Dependency Injection for theme handler. as suggested by tstoeckler in #5.
Comment #45
lokapujyaMaybe someone that understand the task from comment #6 can create an issue for that too.
Comment #46
sunAdding two related issues that are significantly changing the affected code.
Some of the suggested conversions in this issue/patch are probably trivial, but it would probably be a good idea to carefully review all of them to see whether it makes sense to convert them here.
Comment #47
sun42: 2151469-42.patch queued for re-testing.
Comment #49
alansaviolobo CreditAttribution: alansaviolobo commentedComment #52
alansaviolobo CreditAttribution: alansaviolobo commentedComment #53
rpayanmInject service on the class instead.
Inject service...
Inject service...
Inject service...
Inject service...
Typo
Comment #54
JeroenTCreated straight reroll.
Comment #55
JeroenTAddressed all changes as suggested by rpayanm. Patch attached!
Comment #56
JeroenTNote to self: Review patches before submitting them...
Comment #59
JeroenTUpdated BlockFormTest + replaced ThemeHandler with ThemeHandlerInterface in __construct.
Comment #60
andypostPatch looks great but incomplete, there's few references in comments
Comment #61
JeroenTRemoved the remaining list_themes() references + also found and removed a reference to _system_rebuild_theme_data().
Comment #62
andypostLooks good
Comment #63
catchWhen would !$theme occur, this looks like a logic change, not just a cleanup.
Otherwise looks good.
Comment #64
ianthomas_ukGood spot. That must have been a reroll mistake - the line was changed the other way round a few weeks ago. They are effectively identical anyway.
This patch is #61 without that line changed.
Comment #66
rpayanmrerolled...
Comment #67
ianthomas_ukgood reroll, back to rtbc
Comment #70
JeroenTTests pass again, so RTBC
Comment #71
alexpottcan't we inject the service here?
Comment #72
rpayanmComment #74
rpayanmoppss sorry :)
Comment #76
rpayanmummmm.
Comment #78
rpayanmTest...
Comment #80
rpayanmfighting against the Tests...
Comment #81
rpayanmYeah :D
Interdiff #71 - #80
Comment #82
star-szr@rpayanm, nice work. Thank you! What about the one in ThemeAccessCheck? Here are a couple minor coding standards/docs things that can be fixed up:
I think this @param should have the data type.
The description for this param needs to be indented by 2 spaces.
Comment #83
rpayanmNext, ThemeAccessCheck...
Comment #84
rpayanmComment #85
rpayanmTrying with ThemeAccessCheck...
Comment #86
ianthomas_uk#85 is #66 plus the injection that alexpott asked for, so RTBC.
Comment #87
alexpottNow we've injected it we need to actually use it :)
$this->themeHandler
instead of\Drupal::service(...
Comment #88
JeroenTAddressed feedback of alexpott in 87.
Comment #89
JeroenTGo testbot!
Comment #90
JeroenTGo testbot!
Comment #91
JeroenTComment #92
alexpottThis issue is a prioritized change (removing deprecated function use) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 88a6bbc and pushed to 8.0.x. Thanks!
Comment #95
a_thakur CreditAttribution: a_thakur commenteddrupal_theme_access() is still is used.
Created https://www.drupal.org/node/2151469 to get rid of this.
Comment #96
star-szr