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.
Meta Issue: #2205673: [META] Remove all @deprecated functions marked "remove before 8.0"
$ git grep system_rebuild_theme_data
core/includes/bootstrap.inc:278: // system_rebuild_module_data() and system_rebuild_theme_data().
core/includes/theme.maintenance.inc:67: if (!function_exists('system_rebuild_theme_data')) {
core/includes/update.inc:47: $themes = system_rebuild_theme_data();
core/lib/Drupal/Core/Updater/Theme.php:79: system_rebuild_theme_data();
core/modules/locale/locale.compare.inc:129: $theme_data = _locale_translation_prepare_project_list(system_rebuild_theme_data(), 'theme');
core/modules/system/module.api.php:89: * _system_rebuild_theme_data(). A module may implement this hook in order to
core/modules/system/src/Tests/Extension/ModuleHandlerTest.php:238: $themes = system_rebuild_theme_data();
core/modules/system/system.module:740: * @see system_rebuild_theme_data()
core/modules/system/system.module:905:function system_rebuild_theme_data() {
core/modules/update/src/Tests/UpdateContribTest.php:295: $theme_data = system_rebuild_theme_data();
core/modules/update/src/UpdateManager.php:126: $theme_data = system_rebuild_theme_data();
Comment | File | Size | Author |
---|---|---|---|
#74 | 2367747-rebuild-theme-74.patch | 8.89 KB | andypost |
#74 | interdiff.txt | 1.19 KB | andypost |
#73 | 2367747-rebuild-theme-73.patch | 8.85 KB | andypost |
#73 | interdiff.txt | 562 bytes | andypost |
#71 | 2367747-71.patch | 8.91 KB | andypost |
Comments
Comment #1
andypostComment #2
vadim.hirbu CreditAttribution: vadim.hirbu commentedComment #3
Palashvijay4O CreditAttribution: Palashvijay4O commentedA patch .
Comment #5
Palashvijay4O CreditAttribution: Palashvijay4O commentedre-roll with changes suggested by alexpott and vijaycs85 .
Comment #6
vadim.hirbu CreditAttribution: vadim.hirbu commentedRemove usage of system_rebuild_theme_data()
Comment #8
Palashvijay4O CreditAttribution: Palashvijay4O commentedSome minor mistake .
Comment #11
Palashvijay4O CreditAttribution: Palashvijay4O commentedPatch.
Comment #12
Palashvijay4O CreditAttribution: Palashvijay4O commentedComment #14
Palashvijay4O CreditAttribution: Palashvijay4O commentedFinal patch .
Comment #15
tim.plunkettYou'll have to rewrap this to 80 characters.
We must be missing test coverage, because that doesn't work.
Remove this.
This should be ThemeHandlerInterface
Remove the extra space after =
Comment #16
Palashvijay4O CreditAttribution: Palashvijay4O commentedWith changes .
Comment #17
vadim.hirbu CreditAttribution: vadim.hirbu commentedComment #18
andypostWhen you provide a patch, please add the interdiff to follow the changes. See https://www.drupal.org/documentation/git/interdiff
trailing whitespace
remove this extra line
Comment #19
gaurav.pahuja CreditAttribution: gaurav.pahuja commentedRemoved extra space.
Comment #20
andypost#15.2 is lost on last reroll
Comment #21
andypostNo idea about the places left here
Comment #22
Alienpruts CreditAttribution: Alienpruts commentedComment #23
Alienpruts CreditAttribution: Alienpruts commented(tried) to fix #20 and #15
Since we are looking for a function in a class (a method) : changed function_exists() to method_exists().
Novice Sprinter here, btw, be gentle :)
Comment #24
Alienpruts CreditAttribution: Alienpruts commentedComment #25
Wim LeersLooking like a good start, please continue! :)
We don't write the implementation detail in comments, we write the interface, so:
ThemeHandlerInterface::rebuildThemeData()
.The if-test is no longer necessary, because any implementation of
ThemeHandlerInterface
is going to have to implement that method, because it is required by the interface :)Comment #26
Alienpruts CreditAttribution: Alienpruts commentedOk, tnx for the feedback, always good for newbies :)
Will try to get the new patch ASAP, but that might have to wait until I can find a power socket here at DrupalCamp Ghent.
Comment #27
Wim LeersThere are plenty of power sockets in the sprint room on the 4th floor!
Comment #28
Alienpruts CreditAttribution: Alienpruts commentedAdded comments from Wim Leers (# 25)
Let me know if I can be of any service here (or point me to my mistakes :) )
Comment #30
pfrenssenThe patch cannot be applied because the last line of whitespace was accidentally cut from the patch file. A standard diff has 3 lines of context around every code change (3 lines before the change, and 3 lines after the change). The third line would be an empty line, but this is missing from the patch, so the git apply command fails on this.
If you want to know how you can figure out these errors, you can use the venerable "patch" command, since this gives more information. It will show you if there are any problems with the patch:
Uploading fixed patch. If you compare both patches you can see the difference :)
Comment #31
pfrenssenComment #32
Alienpruts CreditAttribution: Alienpruts commentedWow, thnx pfrenssen. Would've taken me forever to figure that one out :) .
Learning a lot these two last days
Comment #33
pfrenssenYou're very welcome @Alienpruts, thanks for helping out!
Comment #34
Wim LeersThis doesn't seem to be used?
Other than that, this patch should convert all other remaining
system_rebuild_theme_data()
calls as well. Do you think you could update those also?Comment #35
Alienpruts CreditAttribution: Alienpruts commentedI will take a stab at it ASAP (meaning probably tomorrow morning :) )
Comment #36
Wim LeersCool, thank you!
Comment #37
rpayanmComment #38
Wim Leers@rpayanm: please don't work on patches that somebody just assigned to him/herself, because that means that other person is working on it. Please remember that next time :)
@Alienpruts: I hope you didn't already begin? In any case, now you can review @rpayanm's patch, which is also good :) Apply his patch, then see if you can find any problems with it: did he forget something? Did he make a typo? If not, you can change the status to RTBC ("Reviewed & Tested by the Community").
Comment #39
rpayanmsorry :( not happen again :)
Comment #40
Wim LeersNo worries :)
Comment #41
Alienpruts CreditAttribution: Alienpruts commented@Wim Leers : no problem
Everything look allright to me, but before I RTBC, one quick something I noticed (again, thank the new IDE for that) :
A quick search around Drupal API reveals that this function is actually deprecated in D8?
https://api.drupal.org/api/drupal/includes!theme.inc/function/list_themes/7
However, another search reveals this :
https://api.drupal.org/api/drupal/core!includes!theme.inc/function/list_...
I have included a final patch incorperating this change for your review :)
Comment #42
Wim LeersGood catch :)
Just one more thing: we can now actually remove
system_rebuild_theme_data()
! :)Comment #43
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedAs per #42, also removed system_rebuild_theme_data() function in this patch.
Comment #45
rpayanmThe patch failed because Drush use system_rebuild_theme_data() for work.
Comment #46
pfrenssenRestoring lost tag.
Comment #47
Alienpruts CreditAttribution: Alienpruts commented@rpayanm, please don't remove tags without a good reason :)
Could you please clarify what you mean by Drush uses system_rebuild_theme_data() to work?
Tnx,
Comment #48
pfrenssenThe testbot uses drush to run the tests. In the process somewhere a call to
system_rebuild_theme_data()
is done, and this causes the test to fail.It seems like this problem has been fixed in this commit from October 4. We need to get drush updated on the testbots. What's the process for this?
Comment #49
pfrenssenCreated issue to update drush on the testbots: #2373319: Update drush.
This issue is blocked on that for the moment.
Comment #50
rpayanmI removed the tag "DrupalCamp Ghent 2014" because the DrupalCamp was celebrated three days ago (Friday, November 7, 2014 - 09:00 to Saturday, November 8, 2014 - 17:00).
Comment #51
pfrenssen@rpayanm, true! But we usually try keeping these sprint tags intact. People that were attending the sprints are still checking these tags regularly to see if there are any updates. Mentors are often also following up on these tags for a few weeks after a sprint has ended to support new contributors. And even months later, it's nice to come across an old issue, see the tag and remember a fun sprint ;)
Comment #52
rpayanm@pfrenssen I got it! :D
Comment #53
Wim LeersWow, I never would've thought we'd be blocked on a drush infra update!
Comment #56
ianthomas_ukThat if statement was included for a reason. I can't be 100% sure from the comment, but I assume that list_themes() calls system_rebuild_theme_data() and there are some circumstances where that happens but the System module hasn't been loaded. If that is the case, then it's presumably no longer true because we're removing all the calls to that function. Maybe it's been refactored in such a way that we no longer need to explicitly load the System module? If not, we need to update the if statement to check for our new dependency, and ideally explain that in the comment. e.g. the previous comment might have read "Ensure system_rebuild_theme_data is available in case it is called by list_themes()".
Switching from list_themes() to the service method is a good change, but it is out of scope for this issue. It is being handled by #2151469: Clean-up usage of deprecated list_themes() and _system_rebuild_theme_data() in favor of theme_handler service (RTBC at the time of writing).
The policy is that we remove the functions themselves in a small followup issue. That makes it easier to revert the removal if there are any problems, and means we're not dependent on drush.
This patch will conflict with #2151469: Clean-up usage of deprecated list_themes() and _system_rebuild_theme_data() in favor of theme_handler service in the first hunk that I quoted above, so I'm postponing on that issue being committed.
Comment #57
ianthomas_ukThis is now unblocked
Comment #58
star-szrWorking on this with some others during sprint weekend in London Canada.
Comment #59
star-szrReroll first, then we will remove the removal of system_rebuild_theme_data() from the patch.
Comment #61
star-szrNot sure how to handle #56.1 so punting that for now. I guess the test failure is because Drush still depends on system_rebuild_theme_data() at this time.
Comment #62
pfrenssenYes this is now postponed on #2404923: Upgrade Drush on qa.d.o.
Comment #63
ianthomas_ukThis isn't postponed, because we don't want to remove the function itself in this issue, so we won't break Drush.
We will need to link the relevant change record though, or write one if one doesn't already exist.
Comment #64
star-szrI agree that this should only remove usages, not the actual function. That is a well-established pattern.
I updated and linked https://www.drupal.org/node/2150863 to this issue, it seemed like a good fit.
Comment #65
a_thakur CreditAttribution: a_thakur commentedLooks good to me. Changing to RTBC.
Comment #66
alexpottThe theme handler should be injected here.
Comment #67
andypostFix #66
Comment #69
andypostmissed hunk, suppose green
Comment #71
andypostI really distracted tonight
Comment #72
star-szrChanges look good, thanks @andypost! :)
I think we should get some closure on #56.1 before commit here.
git blame
points to #766100: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7 which conveniently @andypost worked on - two patches were committed from that issue:First, this one: #766100-12: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7
Then, this one (looks like it just moved the code): #766100-36: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7
Comment #73
andypostLet's try to remove this hunk at all. Looks theme handler does not depends on system module code any-more, so code will use autoloader.
Issue #766100: Undefined function _system_rebuild_theme_data when trying to run update.php from D6 -> D7 is d7 related (we have no other ability to make sure that function available). But d8 use autoloader and container so let's see.
Comment #74
andypostAnd we using full namespaced path for classes in code comments
Comment #75
dawehnerWhat is the reason that we can remove it?
Comment #76
dawehner@andypost explained it to me. We no longer use
system.module
for the theme system, yeah!!Comment #77
star-szrAwesome :) thanks @andypost @dawehner! And appreciate the full namespaces, that was bugging me.
+1 for RTBC.
Comment #78
alexpottCommitted a01453a and pushed to 8.0.x. Thanks!
The beta evaluation is contained in the meta issue.
Comment #80
Wim LeersAwesome — less deprecated stuff :)