API Substitute silverstripe/config module for core config #6641
Conversation
| /** | ||
| * Factory for silverstripe configs | ||
| */ | ||
| class CoreConfigCreator |
There was a problem hiding this comment.
What's the difference between a "factory" and a "creator"? We seem to use both terms in core
| $instance = new CachedConfigCollection(); | ||
|
|
||
| // Set root cache | ||
| $instance->setPool(new FilesystemAdapter('configcache', 0, getTempFolder())); |
There was a problem hiding this comment.
Is there a plan for how to make this more pluggable? I guess it'll be easier once we create an App object as part of main.php bootstrap, where you could manually set services before relying on DI?
There was a problem hiding this comment.
Problem with that is the config is already built before it gets to the users code. You'd need to restructure how SS is bootstrapped.
There was a problem hiding this comment.
There's a little explanation and discussion here: #6252 (comment)
There was a problem hiding this comment.
We do setup .env prior to this stage, so it's possible we could use non-yml/statics to configure this. maybe use getenv?
There was a problem hiding this comment.
The problem is that different backends have different constructors. eg. filesystem has none, redis has url, username, password etc.
There was a problem hiding this comment.
I recommend that we park config-cache-pluggability until after we have an app object.
| * @param string $class | ||
| * @param mixed $options | ||
| * @param callable $next | ||
| * @return string |
There was a problem hiding this comment.
Return string|array|boolean|numeric I assume?
|
@chillu Once silverstripe/silverstripe-config#4 is merged, can you nudge the tests on this and assuming they pass merge it on Monday? |
|
@sminnee I'll do a rebase and do the factory class rename at the same time. |
tractorcow
force-pushed
the
pulls/4.0/micmania-config
branch
from
0e5219d to
0b3b868
Compare
|
Rebased and renamed that class. |
|
Oops, rebase didn't remove Zend_Cache from new ModuleManifest. Fixed now. :D |
|
Need a merge on silverstripe/silverstripe-config#4 then we can restart this build. (after adding to packagist of course). |
tractorcow
force-pushed
the
pulls/4.0/micmania-config
branch
from
df40962 to
e27a5d0
Compare
|
Restarting with fix applied to testsession module |
Fix rebase regressions
tractorcow
force-pushed
the
pulls/4.0/micmania-config
branch
from
a810461 to
40aed4e
Compare
|
Attempting to run with an experimental version of behat-extension to debug out of memory issues. |
|
Please note that I've added a few "experimental" commits that should NOT be merged into the final PR. I'll remove these once I get tests green. |
|
Added a new fix for silverstripe/silverstripe-config#5 |
tractorcow
force-pushed
the
pulls/4.0/micmania-config
branch
from
9e9cc9c to
e74556b
Compare
|
I've added two new bugfix PRs (see first comment) and pushed another change to this PR that should address certain issues. |
|
I've merged config#5 and re-triggered one of the builds on this PR to confirm that it doesn't break anything: https://travis-ci.org/silverstripe/silverstripe-framework/jobs/205966670#L210 |
@tractorcow can you confirm that these have now been removed and you're in principle happy to see this merged? |
|
Yes those are removed. :) |
Implements #6477
Config module work available for review at silverstripe/silverstripe-config#4
Note that this change includes a few other api enhancements and changes designed to support the updated API. This includes a new module registration system (allowing modules to be referenced by full composer name), and changes to Director.
Non-trivial documentation updates will follow.
other PR:
.envfile to dev mode. silverstripe-behat-extension#148 (bugfix for missing.envhandling)