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.
Problem/Motivation
Similar to #2885129: [PHP 7.2] create_function() is deprecated, assuming we do plan to support PHP 7.2, we will need to swap out each() calls in core (and eventually contrib too) with alternative foreach()
or a key()/current()
.
There are many such calls, and I'm not sure a single patch would be ideal or several small ones would be (I personally think the latter).
Ref https://wiki.php.net/rfc/deprecations_php_7_2#each
Proposed resolution
Use foreach()
or a key()/current()
.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#43 | 2885309-43.patch | 9.42 KB | alexpott |
Comments
Comment #2
Ayesh CreditAttribution: Ayesh as a volunteer commentedComment #3
andypostComment #4
klausiReplacing those each() calls with proper foreach() loops is a good novice task :)
Comment #5
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedThis issue is for drupal 7 or 8??
Comment #6
Ayesh CreditAttribution: Ayesh as a volunteer commentedI think we better focus on D8 first, and then backport/fix for D7 too.
Comment #7
andypostComment #8
Ayesh CreditAttribution: Ayesh as a volunteer commentedI will submit a patch for review shortly covering the lines listed above. Thanks Andrey for the list.
Comment #9
Ayesh CreditAttribution: Ayesh as a volunteer commentedComment #10
Ayesh CreditAttribution: Ayesh as a volunteer commentedComment #11
klausiindentation error, make sure this is on the same level as the comment before.
indentation error.
After applying the patch 3 instances are missing:
Comment #12
Ayesh CreditAttribution: Ayesh as a volunteer commentedSorry yes I will get into that. I was trying to get drupal.org CI to run the tests (it takes forever on my computer for some reason today).
Comment #13
Ayesh CreditAttribution: Ayesh as a volunteer commentedComment #14
pk188 CreditAttribution: pk188 at OpenSense Labs commentedFixed 1 & 2 of #11. Patch applying cleanly in my system.
Comment #15
klausiThanks, after applying the patch 3 instances are missing:
Comment #16
arunkumarkHi All,
I have rerolled the patch #14 with fixing the comment #15.
Comment #17
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commented@arunkumark, Next time onwards kindly add interdiff file also with patch.
Here is the minor change.
foreach ($callbacks as $key => $callbacks) {
should be
foreach ($callbacks as $key => $callback) {
Comment #18
pk188 CreditAttribution: pk188 at OpenSense Labs commentedFixed #17.
Comment #19
naveenvalechaThanks! looks good. checked on local. it's not present in any other PHP file.
$ grep --include=\*.php -rnw 'core/' -e 'each('
Comment #21
drewklein CreditAttribution: drewklein commentedThe change to DiffEngine.php will not necessarily work correctly, see https://3v4l.org/DZBS0.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commented@drewklein, good point! And thanks for this information here and your work in #2887904: remove uses of each() function. Can you provide a patch with a more correct version replacements? And interdiff with last patch of current issue.
Also maybe we can use iterator, to reduce the diff with
DiffEngine.php
? Something like https://3v4l.org/aj5JZComment #23
andypostThis change will happen in contrib so better to profile & file change record with suggestions
Comment #24
drewklein CreditAttribution: drewklein commentedHere are the suggested patches and interdiff files.
Comment #25
dawehner@drewklein
Do you think you are able to fix the failures?
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commented#23:
I can not perform qualitative profiling, but simple time measurement (see test.php) gives the following results for array with 1M items:
Intersting fact, when xdebug enabled each() works faster than iterator (or iterator works very slow):
done (if I understand correctly).
#24: thanks!
#25: done.
Comment #27
andypostindeed iterator is slow(
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedIf we use drewklein's idea about
foreach
(#2887904-5: remove uses of each() function), but change the injection to the wrapper condition, will this be a good compromise between minimize the amount of changes, performance and compatibility?Diff with -w flag to avoid noise due to indentations.
Comment #29
drewklein CreditAttribution: drewklein commentedWhat's the status of this change?
Comment #30
drewklein CreditAttribution: drewklein commentedComment #32
drewklein CreditAttribution: drewklein commentedComment #33
vegantriathleteIf you do not feel comfortable digging into the code for this, you could at least create a proper issue summary.
Comment #34
andypostAs #26 shows
foreach
is the most performantNot sure it needs CR besides suggestion to use
foreach
Comment #36
catchPlease open a separate issue for the 7.x backport, the 8.x patch should ideally be the last one on this issue so DrupalCI can retest it easily etc.
Comment #37
andypost@catch d7 patch #30 is hidden in #34, so #28 is lat valid patch
Comment #38
xjmThanks for working on this issue!
I'd add this to the docblock above it rather than adding a separate docblock. After all, it's adding a diff either way.
I think we should also add/enable a coder rule for this to ensure that we don't reintroduce
each()
use.Comment #39
xjmMeant "Needs work", but we can also postpone the issue on adding the coder rule if one is not already available in the Drupal ruleset.
Comment #40
xjmOh, I forgot to mention, I grepped to confirm this is fixing all instances of
each()
once the patch is applied:Comment #41
vegantriathleteComment #42
alexpottWe have Generic.PHP.DeprecatedFunctions sniff but that is unfortunately not picking this up on PHP7.2. I think that is because
each
is a built-in and therefore the way in which it detects deprecated functions does not work for this. We can add each to the list of functions in \Drupal_Sniffs_Functions_DiscouragedFunctionsSniff.Comment #43
alexpottAs noted in #42 I think there is a place to make that change in coder that @xjm has requested but, on second thoughts, I'm not sure we should be adding deprecations of inbuilt functions to our coding standards checks. We don't have checks for all the changes in PHP7 from PHP5 - see http://php.net/manual/en/migration70.incompatible.php#migration70.incomp.... I think what we really need is a testbot for PHP7.2 - if we had that we have a gazillion failing tests and this would be one of the causes.
Wrt to the comment format that @xjm asked for in #38. I think what @vaplas was going for was to copy what is in core/lib/Drupal/Core/Archiver/ArchiveTar.php. That said that file has
// @codingStandardsIgnoreFile
and this does not. There are a couple of other things to add to the list so let's file a followup to address whether a comment needs to be added and just remove the comment altogether. I think there is a argument the comment is unnecessary and we should just be using git for this.Also as this is producing warnings in PHP7.2 and that will out in November this issue is critical.
Comment #44
alexpottFiled a followup for whether we should add a comment - see #2920903: Discuss whether a list of changes for Drupal 8 is needed in Diff class
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks nice! +1 to RTBC
Absolutely true)
Comment #46
mondrakeTried this patch, works fine :), RTBC
However, while this removes calls to each() in Drupal's codebase, PHP Unit tests still fail on PHP 7.2, apparently because PHP Unit itself has calls to each() in the release we can use, 4.8.36.
See https://github.com/sebastianbergmann/phpunit/blob/4.8/src/Util/Getopt.ph...
I checked the 5.7 branch and it looks like the calls to each() were prefixed with the error suppression operator @, see https://github.com/sebastianbergmann/phpunit/commit/20c70e0b72eb92367d00...
EDIT:
There will be no more releases for the 4.8 series, anyway :(, https://github.com/sebastianbergmann/phpunit/pull/2773
Comment #47
borisson_We really should get a php7.2 testbot to verify this, but it looks great. RTBC++
Comment #48
catchCommitted 7206f5d and pushed to 8.5.x. Thanks!
Comment #50
mondrakeShoudn't this be pushed to 8.4.x, too? (assuming we want to support PHP 7.2 for it)
Triggered a test of #43 on 8.4.x.
Comment #51
catchIf we have a 7.2 DrupalCI environment passing tests with 8.5.x, then we could backport these patches then, but that seems pretty far off at the moment so not sure about backports individually.
Comment #52
JayKandariCreated #2925449: [PHP 7] Function each() is deprecated since PHP 7.2 [D7] for D7.
Comment #54
Niktopia CreditAttribution: Niktopia commentedFresh install of Drupal 8.4.4 showing this issue on PHP 7.2.2 resulting in no styling/themes not working.
-- Install Site --
Warning: is_dir(): open_basedir restriction in effect. File(c:\winnt\temp) is not within the allowed path(s): (C:\Windows\Temp\) in Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() (line 37 of core\lib\Drupal\Component\FileSystem\FileSystem.php).
Drupal\Component\FileSystem\FileSystem::getOsTemporaryDirectory() (Line: 595)
system_requirements('install') (Line: 967)
drupal_check_profile('standard') (Line: 1976)
install_check_requirements(Array) (Line: 1028)
install_verify_requirements(Array) (Line: 671)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
-- Configure Site --
Deprecated function: The each() function is deprecated. This message will be suppressed on further calls in Drupal\Core\Extension\ThemeInstaller->install() (line 124 of core\lib\Drupal\Core\Extension\ThemeInstaller.php).
Drupal\Core\Extension\ThemeInstaller->install(Array, 1) (Line: 164)
Drupal\Core\Extension\ThemeHandler->install(Array) (Line: 1563)
install_profile_themes(Array) (Line: 671)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
-- Finish --
Error message
Deprecated function: The each() function is deprecated. This message will be suppressed on further calls in Drupal\Core\Extension\ThemeInstaller->install() (line 124 of core\lib\Drupal\Core\Extension\ThemeInstaller.php).
Drupal\Core\Extension\ThemeInstaller->install(Array, 1) (Line: 164)
Drupal\Core\Extension\ThemeHandler->install(Array) (Line: 1563)
install_profile_themes(Array) (Line: 671)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
Warning: count(): Parameter must be an array or an object that implements Countable in Drupal\Core\Form\FormValidator->doValidateForm() (line 260 of core\lib\Drupal\Core\Form\FormValidator.php).
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'install_configure_form') (Line: 119)
Drupal\Core\Form\FormValidator->validateForm('install_configure_form', Array, Object) (Line: 571)
Drupal\Core\Form\FormBuilder->processForm('install_configure_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('install_configure_form', Object) (Line: 899)
install_get_form('Drupal\Core\Installer\Form\SiteConfigureForm', Array) (Line: 593)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
Warning: count(): Parameter must be an array or an object that implements Countable in Drupal\Core\Form\FormValidator->doValidateForm() (line 260 of core\lib\Drupal\Core\Form\FormValidator.php).
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'install_configure_form') (Line: 119)
Drupal\Core\Form\FormValidator->validateForm('install_configure_form', Array, Object) (Line: 571)
Drupal\Core\Form\FormBuilder->processForm('install_configure_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('install_configure_form', Object) (Line: 899)
install_get_form('Drupal\Core\Installer\Form\SiteConfigureForm', Array) (Line: 593)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
Warning: count(): Parameter must be an array or an object that implements Countable in Drupal\Core\Form\FormValidator->doValidateForm() (line 260 of core\lib\Drupal\Core\Form\FormValidator.php).
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'install_configure_form') (Line: 119)
Drupal\Core\Form\FormValidator->validateForm('install_configure_form', Array, Object) (Line: 571)
Drupal\Core\Form\FormBuilder->processForm('install_configure_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('install_configure_form', Object) (Line: 899)
install_get_form('Drupal\Core\Installer\Form\SiteConfigureForm', Array) (Line: 593)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
Warning: count(): Parameter must be an array or an object that implements Countable in Drupal\Core\Form\FormValidator->doValidateForm() (line 260 of core\lib\Drupal\Core\Form\FormValidator.php).
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object) (Line: 239)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'install_configure_form') (Line: 119)
Drupal\Core\Form\FormValidator->validateForm('install_configure_form', Array, Object) (Line: 571)
Drupal\Core\Form\FormBuilder->processForm('install_configure_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm('install_configure_form', Object) (Line: 899)
install_get_form('Drupal\Core\Installer\Form\SiteConfigureForm', Array) (Line: 593)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)
Comment #55
bkosborneAll - I'm helping with the D7 version of this issue and I think I found a bug with this implementation.
This patch changed the following code in
ModuleInstaller
from:to
By changing the outer loop to a foreach, I believe that means any dependencies that are added to the module list are not checked to see if they exist or not, because the foreach will never get to them. Foreach operates on a copy of the original array. Instead I think what's needed is to manually iterate:
We tried using a foreach for this in the D7 version of the patch but tests were failing.
Comment #56
bkosborne@alexpott found that this is not actually a bug due to how
->requires
works on modules, but the code should be cleaned up anyway. Created follow up: #2950400: Clean up confusing code in ModuleInstaller that checks for dependenciesComment #57
aadeshvermaster@gmail.com CreditAttribution: aadeshvermaster@gmail.com as a volunteer commentedWhen I installing the drupal 8.3.7 on our local machine that has php 7.2, I got this issue on Configuration Site page. After replacing the each loop with foreach loop and install again then i did not get this issue. I have replaced the file in following manner
- while (list($theme) = each($theme_list)) {
+ foreach ($theme_list as $theme => $value) {