Fix config variable mangling #520

Merged
merged 2 commits into from Dec 8, 2016

Projects

None yet

2 participants

@matslindh
Contributor

If a local config file used the same variable internally as the one used in config.default.php, the internal values was overwritten by the name in the local configuration file. This meant we ended up with a cryptic errors about missing routes or other default values that were not initialized as they should have been.

By wrapping the require in a closure, we keep any local variable declared internal to the closure and thus avoid introducing them to the scope of the default configuration file.

@matslindh matslindh Fix config variable mangling
If a local config file used the same variable internally as the one used
in config.default.php, the internal values was overwritten by the name in
the local configuration file. This meant we ended up with a cryptic errors
about missing routes or other default values that were not initialized as
they should have been.

By wrapping the require in a closure, we keep any local variable declared
internal to the closure and thus avoid introducing them to the scope of
the default configuration file.
4a70cf7
@matslindh
Contributor

I'm not sure how we should test this properly, as the behat tests doesn't run on my development computer (and they define the custom imbo config path anyway which skips this part of the code). Any good suggestions?

@matslindh
Contributor

This is a PR to fix #519.

config/config.default.php
@@ -393,24 +393,29 @@
'indexRedirect' => null,
];
+// Keep all external configuration separate
+$extraConfig = array();
@christeredvartsen
christeredvartsen Dec 8, 2016 Member

$extraConfig = [];

@christeredvartsen
Member

One way to test might be to add a custom header to the router (like the custom config file one) that runs the code in config.default.php and that includes a bunch of custom configuration files which share variable names, then simply dumps the configuration instead of running the actual Imbo application...

Not sure how doable this is but I can have a look at it on the work I'm doing in #513.

@christeredvartsen christeredvartsen merged commit 16ed9f1 into imbo:develop Dec 8, 2016

1 check passed

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