Skip to content

Conversation

@cedricziel
Copy link
Member

@cedricziel cedricziel commented Jan 10, 2018

Description

When starting the installer the first time, the current config provided to the deployment config writer is not an array, thus array_replace_recursive emits a warning. With a stricter error handler this will be converted to an exception which prevents running the installer entirely.

This change fixes the behaviour by checking if the current config is present and resumes operation with the already initialized factory config.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jan 10, 2018

CLA assistant check
All committers have signed the CLA.

@orlangur orlangur self-assigned this Jan 10, 2018
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How $currentData equal to null passes condition in line 131?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the description wording to use "not an array".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the static analysis complains. I guess I'll move the condition up to 131.

@orlangur
Copy link
Contributor

When starting the installer the first time, the current config provided to the deployment config writer is not an array, thus array_replace_recursive emits a warning

Could you provide exact steps please? First you mentioned there could be null, what can be there if not array?

@orlangur
Copy link
Contributor

Ping @cedricziel :)

@cedricziel
Copy link
Member Author

Oh! Well, yeah. So... I think there was something wrong with existent but empty config files in terms of a shared config file through deployer. I consider this fix valid nevertheless since an empty config file is still present (but no array though).

@orlangur
Copy link
Contributor

@cedricziel,

-                if ($currentData) {
+                if (is_array($currentData)) {

it was checking for emptiness before your change. So, can situation you described happen in normal circumstances and how? Before applying any fix QAs need to check issue actually occurs on current stable release.

@cedricziel
Copy link
Member Author

I admit this might be an edge case, but better play it safe, no?

@orlangur
Copy link
Contributor

@cedricziel I may be wrong but proposed logic hides problem as to me. So it is a good thing to notify user there is a problem with $currentData (ref: https://en.wikipedia.org/wiki/Fail-fast).

Although, if there is a real-world scenario with such edge case, it needs to be fixed. Let's concentrate on producing exact steps to reproduce first. So, somehow in $currentData we received something not empty but not an array. Was it an object? A string?

@orlangur
Copy link
Contributor

@cedricziel please provide more details, if possible, so that issue can be reproduced on mainline.

@cedricziel
Copy link
Member Author

I'll close this for now - unfortunately I don't have time for this currently.

@cedricziel cedricziel closed this Feb 1, 2018
@orlangur orlangur added this to the January 2018 milestone Feb 1, 2018
@barryvdh
Copy link
Contributor

barryvdh commented Feb 6, 2018

I just ran into this as well.. Same error, same fix.

@orlangur
Copy link
Contributor

orlangur commented Feb 6, 2018

@barryvdh can you elaborate steps to reproduce maybe? This proposed change looks more as a workaround than a proper fix.

@barryvdh
Copy link
Contributor

barryvdh commented Feb 6, 2018

I created a working version on my development machine. Then did a checkout on a different machine and ran the install script (php bin/magento setup:install). It just stopped at step 3:

[Progress: 3 / 473]
Installing deployment configuration...
[Exception]  Warning: array_replace_recursive(): Argument #1 is not an array in
vendor/magento/framework/App/DeploymentConfig/Writer.php on line 135 

I have the modules committed in config.php, but nothing else.

Unfortunately I now realize I'm on 2.2.0, so I cannot really confirm this is still in 2.2.2.

@orlangur
Copy link
Contributor

orlangur commented Feb 6, 2018

@barryvdh no problem with 2.2.0 as this line does not seem to be changed in 2.2.2.

[Exception]  Warning: array_replace_recursive(): Argument #1 is not an array in
vendor/magento/framework/App/DeploymentConfig/Writer.php on line 135 

Could you please var_dump what was there non-empty and non-array? If not, no problem, I'll try to reproduce it with

I have the modules committed in config.php, but nothing else.

today or tomorrow.

@barryvdh
Copy link
Contributor

barryvdh commented Feb 9, 2018

I just setup a new store, same issue. Steps:

  1. Use composer-create to start a new Magento installation, following the dev docs
  2. Install locally (using commandline)
  3. git add ., git commit -am "INitial commit", git push` with default gitignore etc.
  4. Checkout installation on second server, run same install command

Result:

[Progress: 3 / 449]
Installing deployment configuration...
[Exception]
Warning: array_replace_recursive(): Argument #1 is not an array in vendor/magento/framework/App/DeploymentConfig/Writer.php on line 137

var_dump($currentData); gives int(1)

@cedricziel
Copy link
Member Author

In my constellation (with deployer as deployment workflow tool) the config file was initially created empty, since there was none and that produced the issue - i think it's perfectly sane to assume that the config can be empty and anticipate it accordingly (my 2ct).

@barryvdh
Copy link
Contributor

barryvdh commented Feb 9, 2018

Okay I figured it out, my deployment system registers symlinks for <releaseX>/app/etc/env.php to shared/etc/env.php, but upon installation the env.php doesn't yet exist. Only the symlink exists to an empty file. So loading that file will just return 1.

I can kinda see this is an issue with the deployment setup, but it would be an easy fix to check in the DeploymentConfig Reader to see if it's an actual result, otherwise return false.

@barryvdh
Copy link
Contributor

barryvdh commented Feb 9, 2018

I created a Pull Request for this. Similar to this PR, but I think that will handle it at the root of the problem; the Reader. That will prevent issues in other places also.
(Btw, I'm also using Deployer indeed)

@cedricziel
Copy link
Member Author

Much better, thx @barryvdh .

@barryvdh
Copy link
Contributor

barryvdh commented Feb 9, 2018

It might be better to handle this at Deployer perhaps? deployphp/deployer#1540

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants