Refactoring Filter::run() - Changed the way after method runs

[vedavith]

Description
Changed run() method in System/Filters/Filters.php

With current functionality, execution of multi filters are sequential. First it runs all the before method and later afters.
Consider this scenario, If we are walking through 2 doors, we go through door 1 and then door 2 and If we want to walk back, we come through door 2 first and then door 1 and this completely make sense. Similarly, with multiple filters. The last executed before should execute it's after first. It's easy to handle responses if we follow this flow.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

Thank you for contributing.

I think you are correct, the last executed before should execute it's after first.
But this PR changes the behavior, the order of after filters.
It is not a normal refactor, or a bug fix. So I think this PR should go to 4.3 branch.

What do other members think?

[sfadschm]

While the idea is right I would oppose to this change.

I might be overlooking something, but is there any place where we define before and after filters with a single statement? AFAIK, they are defined in two different arrays, so why not just put each set in the order that you want them to be executed?

Anyways, this will likely break a lot of projects, so on first glance, I do not see the benefit.

[vedavith]

Thank you for contributing.

I think you are correct, the last executed before should execute it's after first. But this PR changes the behavior, the order of after filters. It is not a normal refactor, or a bug fix. So I think this PR should go to 4.3 branch.

What do other members think?

Thank you @kenjis

[vedavith]

While the idea is right I would oppose to this change.

I might be overlooking something, but is there any place where we define before and after filters with a single statement? AFAIK, they are defined in two different arrays, so why not just put each set in the order that you want them to be executed?

Anyways, this will likely break a lot of projects, so on first glance, I do not see the benefit.

@sfadschm Yes, there is a place in config where we can do it. But the catch is, It runs for every route we have. By the way, shouldn't it be the default functionality with the framework itself?

[sfadschm]

Can you link me to the config section? I cannot find this functionality.

Honestly, I believe defining these filters individually is the most common use case as usually, you (or at least I😂) do not run the same filters before and after.

The proposed implementation means that everyone who defines the filters as arrays will have to fill the after filters list in reversed order which is quite unintuitive.

[vedavith]

Can you link me to the config section? I cannot find this functionality.

Honestly, I believe defining these filters individually is the most common use case as usually, you (or at least Ijoy) do not run the same filters before and after.

The proposed implementation means that everyone who defines the filters as arrays will have to fill the after filters list in reversed order which is quite unintuitive.

Config/Filters.php - Check line 34. Here we can add our aliases in the order that we want. But, these will run for all the routes we mention filters for

[sfadschm]

Config/Filters.php - Check line 34. Here we can add our aliases in the order that we want. But, these will run for all the routes we mention filters for

Yes, but still, the before and after filters are defined individually?

[vedavith]

Config/Filters.php - Check line 34. Here we can add our aliases in the order that we want. But, these will run for all the routes we mention filters for

Yes, but still, the before and after filters are defined individually?

Correct, even though we define those individually, the flow will be some thing like this. I've just logged the sequence of execution, Instead of handling AuthenticationFilter::after() I want to handle ResponseModifierFilter::after() first.

INFO - 2022-07-13 04:09:20 --> App\Filters\AuthenticationFilter--before
INFO - 2022-07-13 04:09:20 --> App\Filters\ResponseModifierFilter--before
INFO - 2022-07-13 04:09:20 --> App\Controllers\WapService--serviceOne
INFO - 2022-07-13 04:09:20 --> App\Filters\AuthenticationFilter--after
INFO - 2022-07-13 04:09:20 --> App\Filters\ResponseModifierFilter--after

[sfadschm]

Can you add your config where you define the filters?

[MGatner]

I think all the comments here cover it. If I could do Filters over (version 5?) they would be two separate interfaces. before and after are two entirely different events and behaviors that just happen to be lumped together as "Filters" and the only connection is the way we define their configuration (which is also pretty strange).

These behavior changes aren't acceptable even if they do make a bit more sense with the current state of things. If you really want to take a pass at this try defining some new middleware interfaces along with Config\Features::$newFilters and see if you can get them compatible with current filter code (assuming the dev interfaces them).

[lonnieezell]

I will just say that traditional middleware doesn’t work because of auto-routing and not having a good way to define what should run when with those.

…

Sent from my iPhone

On Jul 13, 2022, at 6:49 AM, MGatner ***@***.***> wrote:  I think all the comments here cover it. If I could do Filters over (version 5?) they would be two separate interfaces. before and after are two entirely different events and behaviors that just happen to be lumped together as "Filters" and the only connection is the way we define their configuration (which is also pretty strange). These behavior changes aren't acceptable even if they do make a bit more sense with the current state of things. If you really want to take a pass at this try defining some new middleware interfaces along with Config\Features::$newFilters and see if you can get them compatible with current filter code (assuming the dev interfaces them). — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

[MGatner]

Thanks for pointing that out! By "middleware" I meant Filters, which is essentially CodeIgniter's only middleware (at this point). Some Event triggers function as a pseudo-middleware (post_controller_constructor). But yes: we can't use third-party middleware the same way as other frameworks.

[kenjis]

I don't get why traditional middleware doesn’t work because of auto-routing.
Can someone explain?

[MGatner]

Lonnie will have to specify what he meant by auto-routing, but our biggest hurdle with third-party middleware is that we are not PSR-7 or PSR-15 compliant so unless a middleware class is so generic as to be pretty much useless we won't be compatible 😅

It's still in my list to finish the HTTP package for these PSR classes and release it like our standalone Cache PSR package.

[MGatner]

@vedavith Sorry we have run away with your Pull Request. Due to the unexpected change in behavior this would present we cannot accept this PR. If you have more questions I'm happy to discuss more; if you really, really think this ordering is important start a forum thread and garner support for it. If you're interested in a "fresh take" on filters based on the comments here I'm happy to help, let me know.

[vedavith]

Hi @kenjis @sfadschm @MGatner @lonnieezell ,Thank you for your support. I will go through the filters once again and come up with few more questions. I believe the order that I've mentioned helps us to maintain our code properly and I'm interested in playing around with our framework and try adding some new middleware interfaces. Closing this request.

[kenjis]

I sent PR #7955. Comments are welcome.