Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Change method for merging environment config for packages #6214

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

Closed
wants to merge 1 commit into from

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
Member

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
Member

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
Member

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
Member

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.