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();
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue summary: View changes
vadim.hirbu’s picture

Assigned: Unassigned » vadim.hirbu
Palashvijay4O’s picture

Status: Active » Needs review
FileSize
7.4 KB

A patch .

Status: Needs review » Needs work

The last submitted patch, 3: 2367747-3.patch, failed testing.

Palashvijay4O’s picture

Status: Needs work » Needs review
FileSize
9.18 KB

re-roll with changes suggested by alexpott and vijaycs85 .

vadim.hirbu’s picture

Remove usage of system_rebuild_theme_data()

The last submitted patch, 5: 2367747-5.patch, failed testing.

Palashvijay4O’s picture

FileSize
9.18 KB

Some minor mistake .

The last submitted patch, 6: drupal8-remove_usage_of-2367747-6.patch.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2367747-8.patch, failed testing.

Palashvijay4O’s picture

FileSize
9.23 KB

Patch.

Palashvijay4O’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: 2367747-11.patch, failed testing.

Palashvijay4O’s picture

Status: Needs work » Needs review
FileSize
9.23 KB

Final patch .

tim.plunkett’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/bootstrap.inc
    @@ -275,7 +275,7 @@ function drupal_get_filename($type, $name, $filename = NULL) {
         // If still unknown, retrieve the file list prepared in state by
    -    // system_rebuild_module_data() and system_rebuild_theme_data().
    +    // system_rebuild_module_data() and \Drupal::service('theme_handler')->rebuildThemeData().
    

    You'll have to rewrap this to 80 characters.

  2. +++ b/core/includes/theme.maintenance.inc
    @@ -64,7 +64,8 @@ function _drupal_maintenance_theme() {
    -  if (!function_exists('system_rebuild_theme_data')) {
    +  $theme_handler = \Drupal::service('theme_handler');
    +  if (!function_exists('$theme->rebuildThemeData')) {
    

    We must be missing test coverage, because that doesn't work.

  3. +++ b/core/modules/update/src/UpdateManager.php
    @@ -13,6 +13,8 @@
    +use Drupal\Core\Extension\ThemeHandler;
    

    Remove this.

  4. +++ b/core/modules/update/src/UpdateManager.php
    @@ -64,22 +66,33 @@ class UpdateManager implements UpdateManagerInterface {
    +   * @var \Drupal\Core\Extension\ThemeHandler
    ...
    +   * @param \Drupal\Core\Extension\ThemeHandler $theme_handler
    

    This should be ThemeHandlerInterface

  5. +++ b/core/modules/update/src/UpdateManager.php
    @@ -123,7 +136,7 @@ public function getProjects() {
    +        $theme_data =  $this->themeHandler->rebuildThemeData();
    

    Remove the extra space after =

Palashvijay4O’s picture

Status: Needs work » Needs review
FileSize
8.61 KB

With changes .

vadim.hirbu’s picture

Assigned: vadim.hirbu » Unassigned
andypost’s picture

When you provide a patch, please add the interdiff to follow the changes. See https://www.drupal.org/documentation/git/interdiff

  1. +++ b/core/includes/bootstrap.inc
    @@ -275,7 +275,8 @@ function drupal_get_filename($type, $name, $filename = NULL) {
    +    // system_rebuild_module_data() and ¶
    

    trailing whitespace

  2. +++ b/core/modules/update/src/UpdateManager.php
    @@ -64,22 +65,33 @@ class UpdateManager implements UpdateManagerInterface {
    +   *
    

    remove this extra line

gaurav.pahuja’s picture

Removed extra space.

andypost’s picture

Status: Needs review » Needs work

#15.2 is lost on last reroll

andypost’s picture

No idea about the places left here

$ git grep system_rebuild_theme_data
core/includes/theme.maintenance.inc:67:  if (!function_exists('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() {
Alienpruts’s picture

Assigned: Unassigned » Alienpruts
Issue tags: +DrupalCamp Ghent 2014
Alienpruts’s picture

FileSize
1.86 KB
635 bytes

(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 :)

Alienpruts’s picture

Assigned: Alienpruts » Unassigned
Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +sprint, +php-novice

Looking like a good start, please continue! :)

  1. +++ b/core/includes/bootstrap.inc
    @@ -271,7 +271,8 @@ function drupal_get_filename($type, $name, $filename = NULL) {
    +    // \Drupal::service('theme_handler')->rebuildThemeData().
    

    We don't write the implementation detail in comments, we write the interface, so: ThemeHandlerInterface::rebuildThemeData().

  2. +++ b/core/includes/theme.maintenance.inc
    @@ -64,7 +64,8 @@ function _drupal_maintenance_theme() {
    -  if (!function_exists('system_rebuild_theme_data')) {
    +  $theme_handler = \Drupal::service('theme_handler');
    +  if (!method_exists($theme_handler, '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 :)

Alienpruts’s picture

Assigned: Unassigned » Alienpruts

Ok, 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.

Wim Leers’s picture

There are plenty of power sockets in the sprint room on the 4th floor!

Alienpruts’s picture

Assigned: Alienpruts » Unassigned
Status: Needs work » Needs review
FileSize
1.96 KB
1.38 KB

Added comments from Wim Leers (# 25)

Let me know if I can be of any service here (or point me to my mistakes :) )

Status: Needs review » Needs work

The last submitted patch, 28: remove_usage_of-2367747-28.patch, failed testing.

pfrenssen’s picture

FileSize
1.97 KB

The 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:

$ patch -p1 < remove_usage_of-2367747-28.patch 
patching file core/includes/bootstrap.inc
patching file core/includes/theme.maintenance.inc
patching file core/includes/update.inc
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 44 with fuzz 2.

Uploading fixed patch. If you compare both patches you can see the difference :)

pfrenssen’s picture

Status: Needs work » Needs review
Alienpruts’s picture

Wow, thnx pfrenssen. Would've taken me forever to figure that one out :) .

Learning a lot these two last days

pfrenssen’s picture

You're very welcome @Alienpruts, thanks for helping out!

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.maintenance.inc
@@ -64,11 +64,10 @@ function _drupal_maintenance_theme() {
+  $theme_handler = \Drupal::service('theme_handler');

This 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?

Alienpruts’s picture

Assigned: Unassigned » Alienpruts

I will take a stab at it ASAP (meaning probably tomorrow morning :) )

Wim Leers’s picture

Cool, thank you!

rpayanm’s picture

Status: Needs work » Needs review
FileSize
6.93 KB
Wim Leers’s picture

@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").

rpayanm’s picture

sorry :( not happen again :)

Wim Leers’s picture

No worries :)

Alienpruts’s picture

Assigned: Alienpruts » Unassigned
FileSize
7.11 KB
576 bytes

@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) :

diff --git a/core/includes/theme.maintenance.inc b/core/includes/theme.maintenance.inc
index e40498c..bfd6a4d 100644
--- a/core/includes/theme.maintenance.inc
+++ b/core/includes/theme.maintenance.inc
@@ -63,12 +63,9 @@ function _drupal_maintenance_theme() {

-  // Ensure that system.module is loaded.
-  if (!function_exists('system_rebuild_theme_data')) {
-    $module_handler = \Drupal::moduleHandler();
-    $module_handler->addModule('system', 'core/modules/system');
-    $module_handler->load('system');
-  }
+  $module_handler = \Drupal::moduleHandler();
+  $module_handler->addModule('system', 'core/modules/system');
+  $module_handler->load('system');
 
   $themes = list_themes();

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 :)

Wim Leers’s picture

Status: Needs review » Needs work

Good catch :)

Just one more thing: we can now actually remove system_rebuild_theme_data()! :)

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
7.7 KB
797 bytes

As per #42, also removed system_rebuild_theme_data() function in this patch.

Status: Needs review » Needs work

The last submitted patch, 43: remove_usage_of-2367747-43.patch, failed testing.

rpayanm’s picture

Issue tags: -DrupalCamp Ghent 2014

The patch failed because Drush use system_rebuild_theme_data() for work.

pfrenssen’s picture

Issue tags: +DrupalCamp Ghent 2014

Restoring lost tag.

Alienpruts’s picture

@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,

pfrenssen’s picture

The 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?

pfrenssen’s picture

Status: Needs work » Postponed

Created issue to update drush on the testbots: #2373319: Update drush.

This issue is blocked on that for the moment.

rpayanm’s picture

I 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).

pfrenssen’s picture

@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 ;)

rpayanm’s picture

@pfrenssen I got it! :D

Wim Leers’s picture

Wow, I never would've thought we'd be blocked on a drush infra update!

Status: Postponed » Needs work

The last submitted patch, 43: remove_usage_of-2367747-43.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Postponed
  1. +++ b/core/includes/theme.maintenance.inc
    @@ -63,14 +63,11 @@ function _drupal_maintenance_theme() {
    -  // Ensure that system.module is loaded.
    -  if (!function_exists('system_rebuild_theme_data')) {
    -    $module_handler = \Drupal::moduleHandler();
    -    $module_handler->addModule('system', 'core/modules/system');
    -    $module_handler->load('system');
    -  }
    +  $module_handler = \Drupal::moduleHandler();
    +  $module_handler->addModule('system', 'core/modules/system');
    +  $module_handler->load('system');
     
    -  $themes = list_themes();
    +  $themes = \Drupal::service('theme_handler')->listInfo();
    

    That 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).

  2. +++ b/core/modules/system/system.module
    @@ -894,19 +894,6 @@ function system_rebuild_module_data() {
    -function system_rebuild_theme_data() {
    -  return \Drupal::service('theme_handler')->rebuildThemeData();
    -}
    

    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.

ianthomas_uk’s picture

Status: Postponed » Needs work

This is now unblocked

star-szr’s picture

Issue tags: +SprintWeekend2015

Working on this with some others during sprint weekend in London Canada.

star-szr’s picture

Status: Needs work » Needs review
FileSize
6.64 KB

Reroll first, then we will remove the removal of system_rebuild_theme_data() from the patch.

Status: Needs review » Needs work

The last submitted patch, 59: 2367747-59.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
6.05 KB
797 bytes

Not 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.

pfrenssen’s picture

Status: Needs review » Postponed

Yes this is now postponed on #2404923: Upgrade Drush on qa.d.o.

ianthomas_uk’s picture

Status: Postponed » Needs review
Issue tags: +Needs change record

This 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.

star-szr’s picture

Component: mail system » theme system
Issue tags: -Needs change record

I 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.

a_thakur’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Changing to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/update/src/UpdateManager.php
@@ -123,7 +123,7 @@ public function getProjects() {
-        $theme_data = system_rebuild_theme_data();
+        $theme_data = \Drupal::service('theme_handler')->rebuildThemeData();

The theme handler should be injected here.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.54 KB
7.81 KB

Fix #66

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 67: 2367747-67.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
822 bytes
8.62 KB

missed hunk, suppose green

Status: Needs review » Needs work

The last submitted patch, 69: 2367747-69.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
677 bytes
3.64 KB
8.91 KB

I really distracted tonight

star-szr’s picture

Changes 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

andypost’s picture

FileSize
562 bytes
8.85 KB

Let'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.

andypost’s picture

And we using full namespaced path for classes in code comments

dawehner’s picture

+++ b/core/includes/theme.maintenance.inc
@@ -62,13 +62,6 @@ function _drupal_maintenance_theme() {
-  // Ensure that system.module is loaded.
-  if (!function_exists('system_rebuild_theme_data')) {
-    $module_handler = \Drupal::moduleHandler();
-    $module_handler->addModule('system', 'core/modules/system');
-    $module_handler->load('system');
-  }
-

What is the reason that we can remove it?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@andypost explained it to me. We no longer use system.module for the theme system, yeah!!

star-szr’s picture

Awesome :) thanks @andypost @dawehner! And appreciate the full namespaces, that was bugging me.

+1 for RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a01453a and pushed to 8.0.x. Thanks!

The beta evaluation is contained in the meta issue.

  • alexpott committed a01453a on 8.0.x
    Issue #2367747 by Palashvijay4O, andypost, Alienpruts, Cottser, er....
Wim Leers’s picture

Awesome — less deprecated stuff :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.