DOCS Update configuration docs #268
Conversation
GuySartorelli
force-pushed
the
pulls/5.0/cms5-configuration
branch
2 times, most recently
from
9ca1111
to
b8fedaf
Compare
| Note that while both objects have similar methods the APIs differ slightly. The below actions are equivalent: | ||
| If you do need to modify configuration at run time, you can do so using the following API: | ||
|
|
||
| * `Config::modify()->merge(MyClass::class, 'property', 'newvalue');` or `MyClass::config()->merge('property', 'newvalue')` - merges the new configuration value in with any pre-existing values for this property |
There was a problem hiding this comment.
I don't think we should be including both ways of doing them when they're equivalent.
While it's OK to mention at the start there are 2x different way so to do it, MyClass::config()->merge('property', 'newvalue') is probably the better of the two since it's shorter, and reinforce s the fact you need to have the Configurable trait applied to the class in order for config to work.
Therefore could you please remove any references to Config::modify()->merge(MyClass::class, 'property', 'newvalue'); style setting here and in the rest of this doc
There was a problem hiding this comment.
This goes into "best practices" which will be covered in #269 later on. For now, both of these are valid ways to access the config and both work just fine. The scope of this PR and the issue it relates to is primarily "are the docs factually correct?"
Given that these examples already existed and I've just moved them and clarified them slightly, this suggestion goes out of scope for this PR.
See #212 regarding scope.
There was a problem hiding this comment.
Also, config does work for classes that don't have the Configurable trait. It's recommended to have that trait, but it's not a requirement.
GuySartorelli
force-pushed
the
pulls/5.0/cms5-configuration
branch
from
b8fedaf
to
6823e91
Compare
GuySartorelli
force-pushed
the
pulls/5.0/cms5-configuration
branch
from
6823e91
to
cca64ea
Compare
- This was removed because the 'config' annotation is no longer recommended by Silverstripe. The rule itself was extending a final class from phpstan:^1.12 upwards as well - See: silverstripe/developer-docs#268
Issue