Skip to content
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

InputFilterManager and merging/adding input filters to a base input filter #39

Open
cbichis opened this issue Oct 27, 2021 · 1 comment

Comments

@cbichis
Copy link

cbichis commented Oct 27, 2021

Q A
New Feature yes
RFC no
BC Break no

Summary

Currently there is no out of the box way to construct an Input filter by InputFilterManager by merging multiple Input Filters without having to construct a factory where each of the merged input filters to be requested from IFM first.

So constructing the final IF is now like this:
Fire IFM for each IF which are going to be merged
Merge all IF into final IF

Eg:
$inputFilter = new StockGetCheckInputFilter();
$inputFilter->merge( $container->get('InputFilterManager')->get(OldEntityCheckInputFilter::class) );
$inputFilter->merge( $container->get('InputFilterManager')->get(SaleInputFilter::class) );
$inputFilter->merge( $container->get('InputFilterManager')->get(InstanceInputFilter::class) );

rather I see more appropriate (by example when subclassing InputFilter) to:
construct final IF into IF init, merging here all other IF
Fire just one time the IFM to actually get the instance

We need a mergeByClass functionality to be used on main IF to merge with the rest of IF's and IFM to be fired one time only...


A similar situation is for adding filters, if we are about to set a name to the "sub-inputfilter" we can't unless we provide an instance, not a class as below:

$this->add([
'type' => InstanceInputFilter::class,
'name' => 'instance'
]);

Of course the below example works, but it's just useless complicating situation as would require addition fire of IFM to build the sub-inputfilter...

$this->add($instanceInputFilter, 'instance');

@cbichis cbichis changed the title InputFilterManager and merging multiple input filters InputFilterManager and merging/adding multiple input filters Oct 27, 2021
@cbichis cbichis changed the title InputFilterManager and merging/adding multiple input filters InputFilterManager and merging/adding input filters to a base input filter Oct 27, 2021
@Ocramius
Copy link
Member

Rather than merging, which requires InputFilter to provide insight into its internals (breaks encapsulation), I'd say that input filters should be chained perhaps?

having to construct a factory where each of the merged input filters to be requested from IFM first.

This seems normal/OK? Not sure why that's a problem...

unless we provide an instance, not a class as below:

I didn't understand the phrasing here.

Of course the below example works, but it's just useless complicating situation as would require addition fire of IFM to build the sub-inputfilter...

The input filter manager is totally optional: I personally assemble input filters manually, without any DIC involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants