Part of meta-issue #2002650: [meta, no patch] improve maintainability by removing unused local variables

File /core/includes/bootstrap.inc

As commented by holingpoon This issued may be fixed by related issue #2016629: Refactor bootstrap to better utilize the kernel: Refactor bootstrap to better utilize the kernel. Current bootstrap.inc code has changed drastically. the following list may not apply anymore

  • Line 592: Unused local variable $databases
  • Line 592: Unused local variable $db_prefix
  • Line 592: Unused local variable $drupal_hash_salt
  • Line 592: Unused local variable $config_directories
  • Line 1543: Unused local variable $base_root
  • Line 2308: Unused local variable $conf
  • Line 3027: Unused local variable $key

but this list of unused variables and includes does apply

  • core/includes/bootstrap.inc:148 $reset will be removed due to deprecation in #2573443: Remove conf_path() from core
  • core/includes/bootstrap.inc:236 $original_type
  • core/includes/bootstrap.inc:1025 $key
  • core/includes/bootstrap.inc:7 use statements of DateTimePlus, Environment, ExtensionDiscovery, ApcClassLoader, Response, LanguageInterface
CommentFileSizeAuthor
#54 remove_unused_local-2081137-54.patch813 bytestassilogroeper
#54 interdiff_47-54.txt959 bytestassilogroeper
#1 2081137-remove-unused-variable-databases-from-bootstrap-inc.patch1.6 KBjan.stoeckler
#5 #5-2081137-interdiff-#1-#5.txt1.01 KBjan.stoeckler
#5 #5-2081137-remove-unused-variable-databases-from-bootstrap-inc.patch773 bytesjan.stoeckler
#9 9-2081137-remove-unused-variable-databases-from-bootstrap-inc.patch773 bytesjan.stoeckler
#9 9-2081137-interdiff-#1-#5.txt1.01 KBjan.stoeckler
#15 15-2081137-interdiff-9-15.txt445 bytesjan.stoeckler
#15 15-2081137-remove-unused-variable-databases-from-bootstrap-inc.patch784 bytesjan.stoeckler
#20 2081137-20-remove-unused-variable-databases-from-bootstrap-inc.patch1.39 KBstuti.manandhar
#25 remove-unused-variable-databases-from-bootstrap-inc-2081137-25.patch610 bytesamitgoyal
#35 remove-unused-variable-databases-from-bootstrap-inc-2081137-35.patch679 bytesrgristroph
#39 remove-unused-variable-databases-from-bootstrap-inc-2081137-39.patch774 byteser.pushpinderrana
#39 interdiff-2081137-35-39.txt929 byteser.pushpinderrana
#45 2081137_45.patch471 bytesMile23
#49 interdiff_45-47.txt1.7 KBtassilogroeper
#47 remove_unused_local-2081137-47.patch1.6 KBtassilogroeper
#53 interdiff_47-53.txt1.7 KBtassilogroeper
#53 remove_unused_local-2081137-53.patch107.85 KBtassilogroeper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jan.stoeckler’s picture

Status: Active » Needs review
FileSize
1.6 KB

Removed unused variables as per issue summary, although line numbers do not fully apply anymore due to other changes to the file (i guess)?

Status: Needs review » Needs work
tstoeckler’s picture

Status: Needs work » Needs review
  1. +++ b/core/includes/bootstrap.inc
    @@ -573,7 +573,7 @@ function drupal_settings_initialize() {
    -  global $databases, $cookie_domain, $conf, $db_prefix, $drupal_hash_salt, $base_secure_url, $base_insecure_url, $config_directories;
    +  global $cookie_domain, $conf, $base_secure_url, $base_insecure_url;
    

    This should be left as is, even though PhpStorm reports them as unused. They are declared as global and then a couple lines below settings.php is included which fills these variables with values. In combination the values from settings.php get propagated to the global scope. The patch will definitely fail with this.

  2. +++ b/core/includes/bootstrap.inc
    @@ -2274,7 +2274,7 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    -  global $conf, $config_directories;
    +  global $config_directories;
    

    Same as above. This should not be changed.

I'm wondering if any of the comments around these two should be changed. There are already some, but none that basically say: "Hey there, I know you think this is unused, but PhpStorm is wrong on this one!" Not sure how to really phrase that, though...

tstoeckler’s picture

Status: Needs review » Needs work
jan.stoeckler’s picture

let's try again.

Status: Needs review » Needs work
Issue tags: -Novice
jan.stoeckler’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice
jan.stoeckler’s picture

OK, let's try it without the pound sign in front of the filenames.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Hmm... I didn't know that Drupal.org was so picky about #-signs in file names. The testbot is right, the file can't be found. Weird.

Aynway, the patch looks great. The foreach() is much clearer than the previous weirdness. The bot will set me straight, if this still fails, otherwise this is RTBC.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice
jan.stoeckler’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice
tstoeckler’s picture

Hmm..., that is a legitimate fail.

+++ b/core/includes/bootstrap.inc
@@ -2983,7 +2983,7 @@ function _drupal_shutdown_function() {
-    while (list($key, $callback) = each($callbacks)) {
+    foreach ($callbacks as $callback) {

So it seems the previous and patched lines in this hunk are not functionally equivalent. We could just do a list(, $callback), but that bit of code really doesn't look very nice and I don't get why the foreach doesn't work.

jan.stoeckler’s picture

Status: Needs work » Needs review
FileSize
445 bytes
784 bytes

OK.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

I'd love for some PHP guru to enlighten me regarding this issue, but this is RTBC nevertheless.

David Hernández’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/bootstrap.inc
@@ -2983,7 +2983,7 @@ function _drupal_shutdown_function() {
-    while (list($key, $callback) = each($callbacks)) {
+    while (list(, $callback) = each($callbacks)) {

This pattern of just skipping the first variable in a list does not actually enhance readability - in my, and Dries's, opinion.

xjm’s picture

Title: Remove Unused local variable $databases from /core/includes/bootstrap.inc » Remove unused local variables from /core/includes/bootstrap.inc
Component: other » bootstrap system
Issue summary: View changes
Priority: Normal » Minor

Let's also check the rest of the file and confirm that there are no other unused local variables.

stuti.manandhar’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

Following suggestion in comment #3, I added comments to places where IDEs flagged unused variables.
Also Issue mentioned in comment #18 has been resolved.

chrisfromredfin’s picture

All - I was looking at this with the Code Sprint group in Boston today. We are fairly confident that the reason the test fails on a foreach but not on while/list/each is because the ShutdownFunctionsTest registers a new shutdown function inside of a shutdown function.

If this is expected behavior (i.e. one is allowed to register a shutdown function from inside a shutdown function) then foreach is off limits per the PHP documentation "As foreach relies on the internal array pointer, changing it within the loop may lead to unexpected behavior."

The each() construct doesn't mess around with pointer index setting/re-setting so it correctly iterates through the entire array, even as it changes.

So, if registering a shutdown function from a shutdown function is OK, then we will have to stick with an each() iterator. If it is not OK, then we need to enforce that in drupal_register_shutdown_function().

parthipanramesh’s picture

Status: Needs review » Reviewed & tested by the community

Works fine. Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Seems that cwells's question was not addressed.

sun’s picture

Status: Needs review » Needs work

The code in question in #21 is no longer being touched by this patch.

That said, very well spotted, @cwells! The intended logic of the while loop should not be touched here.

Nevertheless, the latest patch is heavily outdated and needs to be re-rolled.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
610 bytes

Please review updated patch.

Looks like all the unused variables are no longer part of bootstrap.inc file. We just need to mention about the global variables in drupal_settings_initialize() that they are being defined/used in settings.php.

sun’s picture

Status: Needs review » Postponed

That's going to conflict with #2016629: Refactor bootstrap to better utilize the kernel, so postponing on that.

(That said, not sure whether the added comment is really needed...)

mgifford’s picture

Status: Postponed » Needs work
Related issues: +#2016629: Refactor bootstrap to better utilize the kernel

Worth taking out the added comment still?

randy1200’s picture

Checking out to fix.

randy1200’s picture

Assigned: Unassigned » randy1200

Checking out for edit.

holingpoon’s picture

This issued may be fixed by related issue #2016629: Refactor bootstrap to better utilize the kernel. Was reviewing current bootstrap.inc checked out from git repository http://git.drupal.org/project/drupal.git and code has changed drastically. Please review and close this issue.

randy1200’s picture

Assigned: randy1200 » Unassigned

The unused variables from the original writeup are long gone. As a new Drupal developer, this last comment entered at lines 403-404 helped me understand that the remaining variables are used elsewhere. I suggest no further changes required for this patch.

randy1200’s picture

Status: Needs work » Needs review

Setting this to "Needs review" as the only remaining question was about leaving the comment in, which I found helpful.

Status: Needs review » Needs work
rgristroph’s picture

As a result of https://www.drupal.org/node/2016629 the drupal_settings_initialize() function moved into Settings::initialize(). I moved the two clarifying comment lines over to the new place, I think they still make sense in that context.

rgristroph’s picture

Status: Needs work » Needs review
rgristroph’s picture

Issue tags: +SprintWeekend2015
Mile23’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -111,6 +111,8 @@ public static function getAll() {
     // Export these settings.php variables to the global namespace.
+    // Some of the variables initialized here might be unused in this function,
+    // however they are defined in settings.php which is included below.

Inline comments should always wrap at 80, so the added lines here could be added to the end of the previous line.

Also, settings.php is required, not included.

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
774 bytes
929 bytes

Made changes as per #38.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. :-)

alexpott’s picture

This looks like repetition to me. Also we're trying to remove these. I'm not convinced this patch is worth it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
valthebald’s picture

Issue tags: -Novice

Removing Novice tag

Mile23’s picture

As it turns out, NetBeans showed me that there is an unused variable in bootstrap.inc, which isn't slated for deprecation.

So here it is.

This patch applies on its own, and can also apply alongside #39 without conflict (since they change different files). #39 still applies, BTW.

I'd suggest that #39 is out of scope, but might be useful documentation. I'd also suggest that a settings service shouldn't be exporting anything to global, but that's just me.

tassilogroeper’s picture

Issue summary: View changes
Issue tags: +sprint, +Barcelona2015

Working on this on BarcelonaCon 2015. updating Summary

tassilogroeper’s picture

I kinda merged mile23 s commit and updated as in the summery discription.

valthebald’s picture

@tassilogroeper: Please attach an interdiff

tassilogroeper’s picture

FileSize
1.7 KB

@valthebald: interdiff was not worth it. so be aware it may confuse more than it helps ...

Status: Needs review » Needs work

The last submitted patch, 47: remove_unused_local-2081137-47.patch, failed testing.

valthebald’s picture

Status: Needs work » Needs review

testbot hiccup, retrying

Mile23’s picture

Status: Needs review » Needs work

We still have this:

function conf_path($require_settings = TRUE, $reset = FALSE, Request $request = NULL) {

Where $reset is unused in the function. However, conf_path() is deprecated and will be gone soon: #2573443: Remove conf_path() from core

So this would be RTBC except:

+++ b/core/includes/bootstrap.inc
@@ -4,23 +4,17 @@
-use Drupal\Component\Datetime\DateTimePlus;
...
-use Drupal\Component\Utility\Environment;
...
-use Drupal\Core\Extension\ExtensionDiscovery;
...
-use Symfony\Component\ClassLoader\ApcClassLoader;
...
-use Symfony\Component\HttpFoundation\Response;
-use Drupal\Core\Language\LanguageInterface;

These changes are out of scope, though definitely well-intentioned. We have a whole set of issues about removing unused use statements: #2533800: Remove all unused use statements from core

Adding them would require a lot of rerolls of other patches, so we should limit the scope here to removing variables.

tassilogroeper’s picture

@mile: yes, due to function conf_path being deprecated I did not touch it.
patch reuses the unused use statements...

tassilogroeper’s picture

tassilogroeper’s picture

Status: Needs work » Needs review
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Diggit. Thanks, @tassilogroeper.

The last submitted patch, 53: remove_unused_local-2081137-53.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: remove_unused_local-2081137-54.patch, failed testing.

Status: Needs work » Needs review
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Restoring RTBC. Still applies, looks good.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Do we actually have test coverage for drupal_shutdown_function()?

catch’s picture

Status: Needs review » Needs work

And also if the title is 'remove unused variables', it should not include operator strictness changes.

Mile23’s picture

Do we actually have test coverage for drupal_shutdown_function()?

Do you want to make it testable? Because that means making a class called Drupal\Core\Shutdown with a bunch of static methods.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

longwave’s picture

Status: Needs work » Closed (outdated)

Closing as outdated. The three remaining tasks from the IS were dealt with in the following issues and as far I can see there is nothing left to do here:

#2599612: Unused variable in drupal_get_filename()
#2885309: [PHP 7.2] each() function is deprecated
#2533800: Remove all unused use statements from core