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

Only one after filter can be called #853

Closed
neokoenig opened this issue Apr 27, 2018 · 5 comments
Closed

Only one after filter can be called #853

neokoenig opened this issue Apr 27, 2018 · 5 comments
Assignees
Labels
bug

Comments

@neokoenig
Copy link
Member

@neokoenig neokoenig commented Apr 27, 2018

i.e,

component extends="Controller" {
	function config(){
		super.config();

        var filters = filterChain();
        var moreFilters = [
            {
                through="one"
            }, {
                through="two"
            }, {
                through="three", type="after"
            }, {
                through="four", type="after"
            }
        ];
        arrayAppend(filters, moreFilters, true);
        setFilterChain(filters);
	}

	function index(){

	}
	// Before
	private function one(){
		filterTest=[];
		arrayAppend(filterTest, "One");
	}
	private function two(){
		arrayAppend(filterTest, "Two");
	}
	// After
	private function three(){
		arrayAppend(filterTest, "Three"); 
	}
	private function four(){
		arrayAppend(filterTest, "Four");
		writeDump(filterTest);
		abort;
	}
}

four() will never get called. You can call four() from three() though.

@neokoenig neokoenig added the bug label Apr 27, 2018
@neokoenig

This comment has been minimized.

Copy link
Member Author

@neokoenig neokoenig commented Apr 27, 2018

NB: /wheels/tests/controller/filters/filterchain.cfc passes.
So it appears the second filter is added to the chain, just not executed for some reason.

@chapmandu

This comment has been minimized.

Copy link
Member

@chapmandu chapmandu commented May 2, 2018

It appears that after filter three is executed, $performedRenderOrRedirect() is returning true, which break;s the after filter chain.

https://github.com/cfwheels/cfwheels/blob/master/wheels/controller/filters.cfm#L129

In processing.processAction() variables.$instance.response is set every request which will in turn return $performedRenderOrRedirect true (via $performedRender()).

https://github.com/cfwheels/cfwheels/blob/master/wheels/controller/processing.cfm#L80

@chapmandu

This comment has been minimized.

Copy link
Member

@chapmandu chapmandu commented May 2, 2018

What do y'al think of this solution (syntax aside).. basically separate logic for before and after filter types. The after filter now only checks for redirects. It passes all tests but I am conscious of introducing regressions.

local.result = $invoke(method=local.filter.through, invokeArgs=local.filter.arguments);
// If the filter returned false, we skip the remaining filters.
if ((StructKeyExists(local, "result") && !isNull(local.result) && !local.result)) {
	break;
}
if (arguments.type == "before" && $performedRenderOrRedirect()) {
	break;
} else if (arguments.type == "after" && $performedRedirect()) {
	break;
}
@chapmandu

This comment has been minimized.

Copy link
Member

@chapmandu chapmandu commented May 2, 2018

Also, the reason this was not picked up in unit tests is because the tests call the filter functions directly, whereas the bug occurs when filters are called as part of processAction. It would be worthwhile adding filter testing to the processAction test package.

@neokoenig

This comment has been minimized.

Copy link
Member Author

@neokoenig neokoenig commented May 3, 2018

Nicely done.

I don't think changing the after filter to to just check for redirects would be a big deal. Arguably I don't think after filters are actually something people have used a lot simply to due to the slightly inconsistent behaviour, so I'd be surprised if it breaks stuff for anyone, if anything, it might mean a second filter which they thought was being called but wasn't then actually gets called; so works as intended.

I'd like to see optional filter testing to processAction - e.g, let's say you have an action which is behind some simple authentication which is executed by filter: instead of having to mockup an authentication object and inject it just so the action runs, it might be nice to be able to do processAction(includeFilters=true|false|before|after) (defaulting to true) or something.

@chapmandu chapmandu closed this in 558b1a0 May 3, 2018
@chapmandu chapmandu self-assigned this May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.