PHP 5.3 warning for command line install [#776178]

drush si -y default (or minimal)

WD php: Warning: Parameter 3 to install_configure_form() expected to be a reference, value given in drupal_retrieve_form() (line 581 of                 [warning]
/var/www/drupal/includes/form.inc).                                                                                                                              
WD php: Warning: implode(): Invalid arguments passed in form_error() (line 1202 of /var/www/drupal/includes/form.inc).                                  [warning]
WD php: Warning: implode(): Invalid arguments passed in form_error() (line 1202 of /var/www/drupal/includes/form.inc).                                  [warning]
WD php: Warning: implode(): Invalid arguments passed in form_error() (line 1202 of /var/www/drupal/includes/form.inc).                                  [warning]
WD php: Warning: array_merge(): Argument #1 is not an array in install_configure_form_submit() (line 1740 of /var/www/drupal/includes/install.core.inc).[warning]
Parameter 3 to install_configure_form() expected to be a reference, value given in drupal_retrieve_form() (line 581 of                                  [warning]
/var/www/drupal/includes/form.inc).                                                                                                                              
implode(): Invalid arguments passed in form_error() (line 1202 of /var/www/drupal/includes/form.inc).                                                   [warning]
implode(): Invalid arguments passed in form_error() (line 1202 of /var/www/drupal/includes/form.inc).                                                   [warning]
implode(): Invalid arguments passed in form_error() (line 1202 of /var/www/drupal/includes/form.inc).                                                   [warning]
array_merge(): Argument #1 is not an array in install_configure_form_submit() (line 1740 of /var/www/drupal/includes/install.core.inc).                 [warning]

Comment	File	Size	Author
#16	array_by_ref.patch	828 bytes	chx
#16
#13	install-cli-form-776178-13.patch	3.53 KB	David_Rothstein
#13
#11	install-cli-form-756034-v2.patch	3.38 KB	marvil07
#11
#5	byref-on-install.patch	1.05 KB	marvil07
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chx CreditAttribution: chx commented 20 April 2010 at 07:13

PHP 5.3.1 (cli) (built: Feb 2 2010 01:00:04)

drush and core both HEAD.

Comment #2

drewish CreditAttribution: drewish commented 21 April 2010 at 02:52

subscribing. ran into this today.

Comment #3

chx CreditAttribution: chx commented 21 April 2010 at 20:41

Status:

Active

» Closed (duplicate)

#777138: Strict warning: Only variables should be passed by reference in install_settings_form() (line 849 of /includes/install.core.inc)

Comment #4

chx CreditAttribution: chx commented 22 April 2010 at 04:49

Status:

Closed (duplicate)

» Active

Still happens. :(

Comment #5

marvil07 CreditAttribution: marvil07 commented 22 April 2010 at 05:24

Title:	drush si throws warning on site install	» PHP 5.3 warning for command line install
Project:	Drush	» Drupal core
Version:		» 7.x-dev
Component:	Code	» install system
Issue tags:		+PHP 5.3

File	Size
byref-on-install.patch	1.05 KB

I changed the parameters at function definitions to avoid the & on the non-modified values at two functions.

Comment #6

marvil07 CreditAttribution: marvil07 commented 22 April 2010 at 05:25

Status:

Active

» Needs review

Comment #7

moshe weitzman CreditAttribution: moshe weitzman commented 22 April 2010 at 06:05

Status:

Needs review

» Reviewed & tested by the community

Nice debugging.

Comment #8

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 22 April 2010 at 09:14

Status:

Reviewed & tested by the community

» Fixed

Wow. That's esoteric. Nice find!

Committed to HEAD. Thanks!

Comment #9

David_Rothstein CreditAttribution: David_Rothstein commented 22 April 2010 at 18:09

Status:

Fixed

» Needs work

Nice debugging! But the fix isn't correct - why should $form_state and $install_state have a different behavior? With $form_state the similar bug was fixed in the calling code, and we need to do that here as well. Also, the committed patch doesn't fix install_settings_form() which has the same exact bug...

Please review #756034: install.core.inc make impossible to use cli install because of install_settings_form signature which attempts to fix this in the calling code.

The commit here didn't break anything since $install_state wasn't being modified, but the intention is that it can be modified - again, just like $form_state. I'm not sure it's a rule, but it seems like we always use &$form_state in all/most form functions for consistency sake (even when we aren't modifying it), so we should probably do the same thing with $install_state. If so, we should roll this patch back once the other issue gets in.

Comment #10

David_Rothstein CreditAttribution: David_Rothstein commented 22 April 2010 at 18:29

chx also posted #778888: Form functions can't take arguments by reference which is related to this issue.

Comment #11

marvil07 CreditAttribution: marvil07 commented 22 April 2010 at 20:06

Status:

Needs work

» Needs review

File	Size
install-cli-form-756034-v2.patch	3.38 KB

Actually, chx suggest me to review where is the exact problem, because like David suggests the real problem is not on install functions, but install is really big ;-)

So, I tried the patch at #3 in #756034: install.core.inc make impossible to use cli install because of install_settings_form signature, and it also solves the problem, and I think in a cleaner way(aka solving the real problem), so I just rerolled the patch to revert my change and apply his change.

Thanks for taking care of this :-)

Comment #12

22 April 2010 at 21:20

Status:

Needs review

» Needs work

The last submitted patch, install-cli-form-756034-v2.patch, failed testing.

Comment #13

David_Rothstein CreditAttribution: David_Rothstein commented 22 April 2010 at 23:51

Status:

Needs work

» Needs review

File	Size
install-cli-form-776178-13.patch	3.53 KB

Looks good to me - thanks! I think the patch failed to apply because of the line that changed the $Id$ - not sure how that got in there, but here is a reroll that leaves it out.

Comment #14

moshe weitzman CreditAttribution: moshe weitzman commented 23 April 2010 at 05:36

Status:

Needs review

» Reviewed & tested by the community

you guys rock.

Comment #15

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 23 April 2010 at 07:50

Status:

Reviewed & tested by the community

» Fixed

Wow. Great sleuthing, folks!

Committed to HEAD! :D

Comment #16

chx CreditAttribution: chx commented 23 April 2010 at 16:12

Status:

Fixed

» Reviewed & tested by the community

File	Size
array_by_ref.patch	828 bytes

Almost but the comment has a small problem: you are not passing an object by reference that's quite a pointless exercise they are passed around by handle anyways which is 99.99% of the time is good enough. You pass in an array as in the code.