API Major rewrite for inclusion into framework #4
Conversation
API New CachedConfigCollection behaviour API Middleware pattern to allow getClassConfig() to be decorated API Support $class and $name instead of $key for properties API Revendor from micmania1\config to SilverStripe\Config BUG Fix support for before:* / after:* ENHANCEMENT: Allow nulls to replace non-nulls API Make Priority methods static API Added shiv for ->update() legacy method API Drop symfony dependencies to 2.8 API Enable stores and transformers to be constructed as a nested composite object API Enable deferred loading of private-static classes
|
Did you mean to include |
|
The original module had it included. It's for testing only. |
|
I'll take it out; It's probably not needed. :D |
There was a problem hiding this comment.
I've left some comments but overall really happy with the work @tractorcow has done 👍
Could you please update the docs to explain middleware usage?
| } | ||
|
|
||
| // Check cache | ||
| if (isset($this->callCache[$class][$disableFlag])) { |
There was a problem hiding this comment.
I'm not really sure what's going on with $options. $options can be an array so when it is, this will break.
Can you give me some background on why $options was implemented in the way it was? ie. Why can it be an int, true or an array instead of deciding on just one type?
There was a problem hiding this comment.
Also, disableFlag doesn't really tell you what its disabling - you have to look through the rest of the code to figure it out. If its kept (re: comment above) it should probably be renames to disableMiddleware or something similar. If we add disableHistory or something like that it could get confusing.
There was a problem hiding this comment.
For the sake of simplicity, I think I should simply remove the array-form, and keep this as a simple int for flag-assignment, and rename to something that implies it's purpose more succinctly.
There was a problem hiding this comment.
Changed to $excludeMiddleware and phpdoc int|true
| */ | ||
| protected function saveMetadata($class, $metadata) | ||
| { | ||
| if (!$this->trackMetadata) { |
There was a problem hiding this comment.
With regards to the disableFlag we're disabling things two separate ways now. I think this is clearer and more explicit so my preference would be for properties.
There was a problem hiding this comment.
disableFlag is set at-call, though, not a property on an object. I think these are separate types of use cases.
There was a problem hiding this comment.
Actually I'm not sure what you are asking here... bug me on Monday?
| * @param MutableConfigCollectionInterface $theirs | ||
| * @return MutableConfigCollectionInterface | ||
| */ | ||
| public static function merge(array $mine, MutableConfigCollectionInterface $theirs) |
There was a problem hiding this comment.
ahhhh I have a problem with adding static functions that I need to get over. I don't know why I didn't make it static to begin with
| } | ||
| return $otherPatterns !== '*'; | ||
| } | ||
| ); |
There was a problem hiding this comment.
I can't comment on the actual line, but this is for line 431 (the todo). There's no sanitsation for the matching so any document name with strange chars will be passed into preg_replace and may break.
| } | ||
| return $otherPatterns !== '*'; | ||
| } | ||
| ); |
There was a problem hiding this comment.
I can't comment on the actual line, but this is for line 431 (the todo). There's no sanitsation for the matching so any document name with strange chars will be passed into preg_replace and may break. Sorry, I never got around to fixing it.
There was a problem hiding this comment.
Replaced with:
// Replace all `*` with `[^\.][a-zA-Z0-9\-_\/\.]+`, and quote other characters
$patternRegExp = implode(
'[^\.][a-zA-Z0-9\-_\/\.]+',
array_map(
function($part) { return preg_quote($part, '%'); },
explode('*', $pattern)
)
);
|
@tractorcow can you have a look at Michael's feedback on Monday? @micmania1 assuming all is well, can you merge this on Monday? |
|
Fixing up feedback now. |
|
@micmania1 I've written up some nice docs on middleware too. :) All points are addressed (except #4, which we can talk about Monday). I'll push up once I check tests are ok. |
|
@micmania1 feedback fixes pushed up as separate commit (for ease of review ^^) Thanks very much for the review; It's well appreciated friend! |
Middleware documentation Improve regular expression in YamlTransformer
tractorcow
force-pushed
the
pulls/1.0/config-redux
branch
from
2621054 to
a51d1f8
Compare
|
Oh look, it's green. :) |
Framework PR will follow later.
Changes:
API Split ConfigCollectionInterface into mutable / immutable
API New CachedConfigCollection behaviour
API Middleware pattern to allow getClassConfig() to be decorated
API Support $class and $name instead of $key for properties
API Revendor from micmania1\config to SilverStripe\Config
BUG Fix support for before:* / after:*
ENHANCEMENT: Allow nulls to replace non-nulls
API Make Priority methods static
API Added shiv for ->update() legacy method
API Drop symfony dependencies to 2.8
API Enable stores and transformers to be constructed as a nested composite object
API Enable deferred loading of private-static classes