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

[5.8] Read an optional mix asset url from MIX_ASSET_URL env variable #28952

Merged
merged 4 commits into from Jun 26, 2019

Conversation

Projects
None yet
5 participants
@themsaid
Copy link
Member

commented Jun 25, 2019

No description provided.

@GrahamCampbell GrahamCampbell changed the title Read an optional mix asset url from MIX_ASSET_URL env variable [5.9] Read an optional mix asset url from MIX_ASSET_URL env variable Jun 25, 2019

@GrahamCampbell GrahamCampbell changed the title [5.9] Read an optional mix asset url from MIX_ASSET_URL env variable [5.8] Read an optional mix asset url from MIX_ASSET_URL env variable Jun 25, 2019

@danilopinotti

This comment has been minimized.

Copy link

commented Jun 25, 2019

When caching the configs by artisan command, the env helper does not always return null?
I believe the most ideal is to put this 'namespace' in a configuration file where it uses the env helper

themsaid added some commits Jun 25, 2019

@themsaid

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

@danilopinotti thanks, did that :)

@X-Coder264

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

IMO, there should be tests for when the repository actually returns some value for the app.mix_url key. Currently all the mock ensures is that the get method gets called with the proper key (but it doesn't return anything).

@laurencei

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Does it have to go into the app.php config file? That is rapidly becoming a "dump all" place to put things.

Wouldnt the view.php config file make more sense in this case? or even create a mix.php file for this plus any future mix related config items?

@taylorotwell taylorotwell merged commit 7b2ba16 into laravel:5.8 Jun 26, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.