Skip to content

Conversation

RSully
Copy link
Contributor

@RSully RSully commented Oct 24, 2014

Right now packages' configurations are merged using array_merge instead of mergeEnvironment. This is not what I would expect.

If there is a specific reason for this, I would love the discussion, but this does not break any unit tests (perhaps that is a separate issue).

Right now packages' config is merged using `array_merge` instead of
`mergeEnvironment` as I would have expected.

If there is a specific reason for this, I would love the discussion
but this does not break any unit tests either.
@GrahamCampbell
Copy link
Collaborator

Maybe this would be better targeted at 5.0 since it's a change in behaviour?

@crynobone
Copy link
Member

#5531 It was merged and reverted before.

@RSully
Copy link
Contributor Author

RSully commented Oct 25, 2014

To give an idea of why I thought this change should be made:

app/packages/{vendor}/{package}/config.php:

<?php

return [
    'abc' => [
        'test' => [
            'title' => 'a',
            'config' => [],
        ]
    ]
]

app/packages/{vendor}/{package}/local/config.php:

<?php
return [
    'abc' => [
        'test' => [
            'config' => [
                'host' => 'thing',
            ]
        ]
    ]
]

When I call Config::get('package::abc') from my local environment I would expect:

[
    'test' => [
        'title' => 'a',
        'config' => [
            'host' => 'thing',
        ]
    ]
]

@RSully
Copy link
Contributor Author

RSully commented Oct 25, 2014

Also, it appears the pull-request you linked to changed both occurrences of array_merge whereas mine only changes the second. I think this would give better behavior, but should be left to discussion here to determine if it is inconsistent.

@GrahamCampbell
Copy link
Collaborator

This needs to go to 5.0 if at all.

@RSully
Copy link
Contributor Author

RSully commented Oct 25, 2014

OK well don't you think it deserves the discussion here then for that decision? Environment configs are rather non-intuitive for packages right now.

@GrahamCampbell
Copy link
Collaborator

Yes, this has already been discussed, and it cannot be changed in laravel 4.2. If it is going to be changed, it MUST be changed in laravel 5.

@RSully
Copy link
Contributor Author

RSully commented Oct 25, 2014

Right, but I mean the discussion as to should/will it be changed in Laravel 5?

@GrahamCampbell
Copy link
Collaborator

Feel free to send a pull to laravel 5, or have a chat to taylor on irc if he has a min.

@RSully RSully deleted the patch-env_config branch June 19, 2015 19:16
@RSully RSully restored the patch-env_config branch June 19, 2015 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants