-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Initial Service Manager v4 Support #136
Conversation
Signed-off-by: George Steel <[email protected]>
…\Filter namespace Signed-off-by: George Steel <[email protected]>
- Drop inclusion of i18n filters - Drop legacy SMv2 validation Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
…ires options. `AbstractPluginManager::get()` no longer has an `options` parameter. Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
…uration variables from abstract plugin manager Signed-off-by: George Steel <[email protected]>
- Remove fragile and now incorrect examination if plugin manager internals - Fix configuration where an alias will no longer resolve because the plugin manager does not self-define all known filters Signed-off-by: George Steel <[email protected]>
Wires up factories and aliases as configured in the package ConfigProvider so that the created plugin manager is functional. Signed-off-by: George Steel <[email protected]>
…n service manager. The test trait shipped on service manager cannot be used because it is designed for `SingleInstancePluginManager` which `FilterPluginManager` cannot inherit from. Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
Signed-off-by: George Steel <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Like to see first plugin manager to not implement the AbstractSingleInstancePluginManager
class as we do have to allow callable
and FilterInterface
types.
However, I would keep the plugin manager configuration in the plugin manager to prevent too much of a BC break. Especially a break which is not really necessary IMO.
I wonder if we should provide a |
I was thinking the same thing. Probably worth exploring in a separate patch |
To reduce BC breaks, retain the concept of default configuration of the plugin manager, moving configuration out of the ConfigProvider and into a class constant. Signed-off-by: George Steel <[email protected]>
@boesing - I'd appreciate another review when you get a chance! Cheers. |
will do later, have to take a 4h train and thus need some stuff to do 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few questions but overall pretty neat work.
… to descendants of `FilterInterface` Signed-off-by: George Steel <[email protected]>
TYVM @boesing 🙏 |
Provides initial support for SMv4
There is still a fair bit of work to do where the FilterPluginManager is consumed by FilterChain + other filters. These will need to be refactored with the FilterPluginManager as a hard-dependency in the constructor, otherwise none of the aliases will work now they have been extracted to the config provider.
Closes #125