Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 $databasesLine 592: Unused local variable $db_prefixLine 592: Unused local variable $drupal_hash_saltLine 592: Unused local variable $config_directoriesLine 1543: Unused local variable $base_rootLine 2308: Unused local variable $confLine 3027: Unused local variable $key
but this list of unused variables and includes does apply
- core/includes/bootstrap.inc:148
$resetwill 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
Comments
Comment #1
jan.stoecklerRemoved unused variables as per issue summary, although line numbers do not fully apply anymore due to other changes to the file (i guess)?
Comment #3
tstoecklerThis 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.
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...
Comment #4
tstoecklerComment #5
jan.stoecklerlet's try again.
Comment #7
jan.stoeckler#5: #5-2081137-remove-unused-variable-databases-from-bootstrap-inc.patch queued for re-testing.
Comment #9
jan.stoecklerOK, let's try it without the pound sign in front of the filenames.
Comment #10
tstoecklerHmm... 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.
Comment #12
jan.stoeckler#9: 9-2081137-remove-unused-variable-databases-from-bootstrap-inc.patch queued for re-testing.
Comment #14
tstoecklerHmm..., that is a legitimate fail.
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.Comment #15
jan.stoecklerOK.
Comment #16
tstoecklerI'd love for some PHP guru to enlighten me regarding this issue, but this is RTBC nevertheless.
Comment #17
David Hernández CreditAttribution: David Hernández commented#15: 15-2081137-remove-unused-variable-databases-from-bootstrap-inc.patch queued for re-testing.
Comment #18
alexpottThis pattern of just skipping the first variable in a list does not actually enhance readability - in my, and Dries's, opinion.
Comment #19
xjmLet's also check the rest of the file and confirm that there are no other unused local variables.
Comment #20
stuti.manandhar CreditAttribution: stuti.manandhar commentedFollowing suggestion in comment #3, I added comments to places where IDEs flagged unused variables.
Also Issue mentioned in comment #18 has been resolved.
Comment #21
chrisfromredfinAll - 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().
Comment #22
parthipanramesh CreditAttribution: parthipanramesh commentedWorks fine. Thanks!
Comment #23
webchickHm. Seems that cwells's question was not addressed.
Comment #24
sunThe 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.
Comment #25
amitgoyal CreditAttribution: amitgoyal commentedPlease 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.
Comment #26
sunThat'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...)
Comment #27
mgiffordWorth taking out the added comment still?
Comment #28
randy1200 CreditAttribution: randy1200 commentedChecking out to fix.
Comment #29
randy1200 CreditAttribution: randy1200 commentedChecking out for edit.
Comment #30
holingpoon CreditAttribution: holingpoon commentedThis 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.Comment #31
randy1200 CreditAttribution: randy1200 commentedThe 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.
Comment #32
randy1200 CreditAttribution: randy1200 commentedSetting this to "Needs review" as the only remaining question was about leaving the comment in, which I found helpful.
Comment #35
rgristroph CreditAttribution: rgristroph commentedAs 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.
Comment #36
rgristroph CreditAttribution: rgristroph commentedComment #37
rgristroph CreditAttribution: rgristroph commentedComment #38
Mile23Inline 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.
Comment #39
er.pushpinderrana CreditAttribution: er.pushpinderrana at Publicis Sapient commentedMade changes as per #38.
Comment #40
Mile23Thanks. :-)
Comment #41
alexpottThis looks like repetition to me. Also we're trying to remove these. I'm not convinced this patch is worth it.
Comment #42
alexpottComment #44
valthebaldRemoving Novice tag
Comment #45
Mile23As 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.
Comment #46
tassilogroeper CreditAttribution: tassilogroeper commentedWorking on this on BarcelonaCon 2015. updating Summary
Comment #47
tassilogroeper CreditAttribution: tassilogroeper commentedI kinda merged mile23 s commit and updated as in the summery discription.
Comment #48
valthebald@tassilogroeper: Please attach an interdiff
Comment #49
tassilogroeper CreditAttribution: tassilogroeper commented@valthebald: interdiff was not worth it. so be aware it may confuse more than it helps ...
Comment #51
valthebaldtestbot hiccup, retrying
Comment #52
Mile23We 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 coreSo this would be RTBC except:
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.
Comment #53
tassilogroeper CreditAttribution: tassilogroeper commented@mile: yes, due to
function conf_path
being deprecated I did not touch it.patch reuses the unused use statements...
Comment #54
tassilogroeper CreditAttribution: tassilogroeper commentedsorry patchfoo was not strong on #53
Comment #55
tassilogroeper CreditAttribution: tassilogroeper commentedComment #56
Mile23Diggit. Thanks, @tassilogroeper.
Comment #61
Mile23Restoring RTBC. Still applies, looks good.
Comment #62
catchDo we actually have test coverage for drupal_shutdown_function()?
Comment #63
catchAnd also if the title is 'remove unused variables', it should not include operator strictness changes.
Comment #64
Mile23Do you want to make it testable? Because that means making a class called
Drupal\Core\Shutdown
with a bunch of static methods.Comment #73
longwaveClosing 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