fixed hiera merge bug

[c33s]

see #434

[bastelfreak]

The next release is probably a major one, we could already drop the old behavior now without all the deprecation boilerplate. I'm a bit worried that we will forget to remove it in the future if we don't do it now. That happened multiple times in the past.

[c33s]

@bastelfreak rebased on master and removed deprecation notices (i am clearly against removing the bc layer).

[bastelfreak]

Hi @voxpupuli/collaborators, I would like to get some feedback on this. We normally don't add a layer of warnings for future breaking changes. My opinion:

• this adds extra complexity that can introduce bugs
• it's very noisy
• We tend to forget things

I don't want to introduce breaking changes in a minor release. Also they always need to be documented clearly in the CHANGELOG.md and README.md. I think it's fine to have a clearly documented breaking change in the CHANGELOG.md if it's a major release and it doesn't happen on each release. That should not require a deprecation layer.

[c33s]

@bastelfreak maybe move this discussion it its own ticket #450 and merge this for now.

[zachfi]

I'm not clear why we rename the variables here instead of using the class parameters as intended. Perhaps the other issues have the context.

[c33s]

@xaque208 it is not about this PR please go to #450 in this PR i removed the deprecation warnings. the variable names are a complete different thing.

so at all who come to this ticket because of the @ voxpupuli/collaborators (don't want to notify again) please head to #450,

the code before the discussion of deprecation notices and BC breaks came up looked like this:

init.pp

...snip...
  Boolean $force_old_merge_behavior               = true,
) inherits ::php::params {

  $real_fpm_package = pick($fpm_package, "${package_prefix}${::php::params::fpm_package_suffix}")

  if ($force_old_merge_behavior) {
    deprecation('php_hiera_manual_lookup', 'the default deep merge behavior is a bug and will be removed in the next major release')

    # Deep merge global php settings
    $real_settings = deep_merge($settings, lookup('php::settings', {value_type => Hash, merge => 'deep', default_value => {}}))

    # Deep merge global php extensions
    $real_extensions = deep_merge($extensions, lookup('php::extensions', {value_type => Hash, merge => 'deep', default_value => {}}))

    # Deep merge fpm_pools
    $real_fpm_pools = deep_merge($fpm_pools, lookup('php::fpm_pools', {value_type => Hash, merge => 'deep', default_value => {}}))

    # Deep merge fpm_global_pool_settings
    $real_fpm_global_pool_settings = deep_merge($fpm_global_pool_settings, lookup('php::fpm_global_pool_settings', {value_type => Hash, merge => 'deep', default_value => {}}))
  } else {
    $real_settings = $settings

    $real_extensions = $extensions

    $real_fpm_pools = $fpm_pools

    $real_fpm_global_pool_settings = $fpm_global_pool_settings
  }
...snip...

[c33s]

@bastelfreak any reason why you don't merge it?

[bastelfreak]

on stuff that's maybe controversial we want to wait at least 3 days (https://github.com/voxpupuli/plumbing/blob/master/share/governance.md). I want to merge it tomorrow if no other collaborator raises concerns.

[c33s]

@bastelfreak is it controversal? i removed the deprecation stuff and only the fix is left. for the controversal stuff i opened up #450