WIP: Merging environment configs deeply or recursively

[claytonrcarter]

This PR aims to alter how configurations are merged so that the process feels a bit more intuitive and requires less duplication of config settings between environments. The docs lay out how configurations can be merged, and this works fine for top-level key/value pairs. I ran into an issue, however, when I wanted to add a filter to a collection so that only posts w/ a published metadata value were included in my production build. Doing so was easy, and just involved the following:

// config.production.php
<?php

return [
    'collections' => [
        'posts' => [
            'filter' => function ($item) {
                return $item->published;
            }
        ],
    ],
];

However this resulted in the rest of the collections settings from my base config.php being discarded. This is because the current implementation simply does a collect()->merge(), which seems to only consider the top-level key/value pairs.

The code I'm submitting alters the way that configs are merged to use array_replace_recursive, which should crawl through the entire config arrays (including nested arrays and Collections) and only replace the key/value pairs that are different, keeping any that have not been altered.

Example:

// config.php
return [
    'baseUrl' => '',
    'production' => false,
    'siteName' => 'Blog Starter Template',
    'siteAuthor' => 'Author Name',

    // collections
    'collections' => [
        'posts' => [
            'author' => 'Author Name',
            'sort' => '-date',
            'path' => 'blog/{filename}'
        ],
    ]
];

// config.production.php
return [
    'baseUrl' => 'https://my-jigsaw-blog.com',
    'production' => true,
    'collections' => [
        'posts' => [
            'filter' => function ($item) {
                return $item->published;
            }
        ],
    ],
];

// merged config (current behavior)
// Note how collections.posts does not contain author, sort or path
[
    'baseUrl' => 'https://my-jigsaw-blog.com',
    'production' => true,
    'siteName' => 'Blog Starter Template',
    'siteAuthor' => 'Author Name',

    // collections
    'collections' => [
        'posts' => [
            'filter' => function ($item) {
                return $item->published;
            }
        ],
    ]
];

// merged config (new behavior)
// Note how collections.posts contains author, sort, path and filter
[
    'baseUrl' => 'https://my-jigsaw-blog.com',
    'production' => true,
    'siteName' => 'Blog Starter Template',
    'siteAuthor' => 'Author Name',

    // collections
    'collections' => [
        'posts' => [
            'author' => 'Author Name',
            'sort' => '-date',
            'path' => 'blog/{filename}',
            'filter' => function ($item) {
                return $item->published;
            }
        ],
    ]
];

Current issues needing advice:

• the config.php used for testing includes a function defn that triggers an error if that config is loaded more than once. As such, each individual test in the project passes but the overall suite fails b/c I needed/wanted to use that config for testing and so it's loaded in 2 different tests, which triggers the "Fatal error: cannot redeclare ...". I would like advice from project maintainers as to whether that function is important to the tests (ie it's presense is testing something), or just something incidental that could be refactored so that that loading config.php in different tests doesn't cause PHP to kvetch.
• if users are depending on the current behavior to discard values in "subarrays", this could introduce a breaking change
• I was aiming for minimal API changes, but I don't love some of the changes I made to achieve that. For example, making mergeConfigs() a static method seems to make sense, but then doing (new static(''))->convertStringCollectionsToArray(...) in that method in order to reuse the convertStringCollectionsToArray() instance method seems hacky and gross. I would also love some advice or suggestions on how to clean that up without making too big of a mess.

Because of these "issues", I've marked this PR as a "WIP". Other than these, though, it's fully functional.

Thanks so much for the great project! I hope that you'll find this change useful.

[damiani]

Great!

RE: your questions...

• That parseDate function in config.php can just be inlined to the date_formatted helper:

'date_formatted' => function ($post) {
    $date = DateTime::createFromFormat('U', $post['date']);

    return sprintf(
        '%s/%s/%s',
        $date->format('m'),
        $date->format('d'),
        $date->format('Y')
    );
},

• Yes, this would be a breaking change, so we'll need to do a point release after merging
• I'll take a look and let you know my thoughts on improving the code!

[damiani]

Just a quick update, we're gonna hold off on merging this for now since it would be a breaking change, but I expect to pull this into v2.0.

[husseinalhammad]

Love this.

Just thinking out loud here; instead of introducing a breaking change, would it be possible to make this optional in v1.x?

Perhaps it can be an option you have to enable in config.production.php:

return [
    'production' => true,
    'recursive' => true,
];

[damiani]

@claytonrcarter and @husseinalhammad Thanks for this! It's definitely a good idea, and the optional flag in config is a great solution. @bakerkretzmar has combined these recommendations into #648, so I'm gonna merge that in.

I appreciate your good thinking and work on this feature!