Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursively add configuration options #962

Merged
merged 3 commits into from
Jan 23, 2017
Merged

Conversation

mbrodala
Copy link
Contributor

@mbrodala mbrodala commented Jan 18, 2017

Q A
Bug fix? Yes
New feature? Yes
BC breaks? Maybe
Deprecations? No
Fixed tickets N/A

This ensures that something like the following works:

set('top', [
  'first' => [
    'second' => [
      'value1',
      'value2',
    ],
  ],
]);

add('top', [
  'first' => [
    'second' => [
      'value3',
    ],
  ],
]);

/*
The following should yield:

Array
(
    [first] => Array
        (
            [second] => Array
                (
                    [0] => value1
                    [1] => value2
                    [2] => value3
                )
        )

)
*/

print_r(get('top'));

@antonmedv
Copy link
Member

Please, add test for this.

@mbrodala
Copy link
Contributor Author

I will. :-)

@mbrodala
Copy link
Contributor Author

@Elfet Can you have another look?

@antonmedv antonmedv merged commit 84ca336 into deployphp:master Jan 23, 2017
@mbrodala
Copy link
Contributor Author

Thanks a lot. :-)

@mbrodala mbrodala deleted the patch-1 branch January 23, 2017 11:07
@thadin
Copy link

thadin commented Feb 7, 2017

This breaks recipes that use strings in their config, like rsync:

set('rsync', [
    'exclude' => [
        '.git',
        'deploy.php',
    ],
    'exclude-file' => false,
    'include' => [],
    'include-file' => false,
    'filter' => [],
    'filter-file' => false,
    'filter-perdir' => false,
    'flags' => 'rz',
    'options' => ['delete'],
    'timeout' => 60,
]);

add('rsync', [
    'exclude' => [
        'uploads'
    ],
    'flags' => 'rzc'
]);

The function array_merge_recursive merges 2 strings with the same key into an array, as a result my flags become ['rz', 'rzc'], which causes the rsync commando to have the arguments -Array (due to the toString conversion.

@mbrodala
Copy link
Contributor Author

mbrodala commented Feb 7, 2017

@thadin You are absolutely right, nice catch. I'll make a PR with a possible fix.

@mbrodala
Copy link
Contributor Author

mbrodala commented Feb 7, 2017

Short status message: array_merge_recursive fails on overriding scalar values, array_replace_recursive fails at keeping numerical keys. The latter is essential e.g. for exclude in the config above.

@thadin
Copy link

thadin commented Feb 7, 2017

One solution could be to create a custom method that loops (recursively) through the options and merges arrays while overriding scalars, but this might cause unexpected behavior for users.

@antonmedv
Copy link
Member

antonmedv commented Feb 7, 2017

/**
* array_merge_recursive does indeed merge arrays, but it converts values with duplicate
* keys to arrays rather than overwriting the value in the first array with the duplicate
* value in the second array, as array_merge does. I.e., with array_merge_recursive,
* this happens (documented behavior):
*
* array_merge_recursive(array('key' => 'org value'), array('key' => 'new value'));
*     => array('key' => array('org value', 'new value'));
*
* array_merge_recursive_distinct does not change the datatypes of the values in the arrays.
* Matching keys' values in the second array overwrite those in the first array, as is the
* case with array_merge, i.e.:
*
* array_merge_recursive_distinct(array('key' => 'org value'), array('key' => 'new value'));
*     => array('key' => array('new value'));
*
* Parameters are passed by reference, though only for performance reasons. They're not
* altered by this function.
*
* @param array $array1
* @param array $array2
* @return array
* @author Daniel <daniel (at) danielsmedegaardbuus (dot) dk>
* @author Gabriel Sobrinho <gabriel (dot) sobrinho (at) gmail (dot) com>
*/
function array_merge_recursive_distinct ( array &$array1, array &$array2 )
{
  $merged = $array1;

  foreach ( $array2 as $key => &$value )
  {
    if ( is_array ( $value ) && isset ( $merged [$key] ) && is_array ( $merged [$key] ) )
    {
      $merged [$key] = array_merge_recursive_distinct ( $merged [$key], $value );
    }
    else
    {
      $merged [$key] = $value;
    }
  }

  return $merged;
}

Use this.

@mbrodala
Copy link
Contributor Author

mbrodala commented Feb 7, 2017

@Elfet That one also doesn't work, I now went with the one mentioned here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants