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

collect config during initial creation #467

Merged
merged 5 commits into from
May 7, 2020
Merged

collect config during initial creation #467

merged 5 commits into from
May 7, 2020

Conversation

reed-jones
Copy link
Contributor

I noticed the config was initially being created as an array in ConfigFile, however in some places it was being used as a Collection instead (the Constructor for CollectionRemoteItemLoader). I previously fixed a very similar bug #423 and was tempted to remove the typehint again, but after a little digging I found what what happening.

in BuildCommand->includeEnvironmentConfig It actually overwrites the original array config with a new collection config. This fix just makes it a collection from the start, (and undoes my previous typehint removal)

@damiani
Copy link
Contributor

damiani commented May 6, 2020

Great idea, though I think it would be preferable to make the conversion directly in the ConfigFile constructor, rather than at the point where it's bound to the container as the config key. So, line 16 of ConfigFile could change to:

-   $this->config = array_merge($config, $helpers);
+   $this->config = collect(array_merge($config, $helpers));

... and then, update any places where can use collection access instead or array access (e.g. line 22 of the same file:)

-   $collections = Arr::get($this->config, 'collections');
+   $collections = $this->config->get('collections');

Finally, it would be good to audit any other uses of ConfigFile->config, and typehint them as Collection for consistency.

Comment on lines +25 to +27
public function __construct(Factory $viewFactory, BladeCompiler $bladeCompiler, Collection $config = null)
{
$this->config = collect($config);
$this->config = $config ?? collect();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure what the most idiomatic way to convert this from an array => collection was with the default value. As far as I can tell the only place its ever called without passing a config is in the ViewRendererTest.php with it_does_not_register_view_hint_paths_if_not_specified_in_config I just didn't want to remove it incase I missed something.

@damiani
Copy link
Contributor

damiani commented May 7, 2020

@reed-jones This is great, I made a couple other adjustments. Thanks for this!

@damiani damiani merged commit e83b80d into tighten:master May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants