Skip to content

Conversation

@dcaswel
Copy link
Contributor

@dcaswel dcaswel commented Aug 26, 2020

Expands the fix of #3085

Description
Filters have the ability to alter the request like adding a user object from an auth filter for example. When it does that the request gets altered implicitly by the filter because the filter's request is pointing to the same object as the request property of the Codeigniter object. If the request gets altered, the filters also return the request into the $possibleRedirect variable. This change makes the update to the request property more explicit by setting the property with what gets returned from the filters. This ensures that if for some reason the object pointers get messed up, the request used by the controllers is the altered one.

@dcaswel
Copy link
Contributor Author

dcaswel commented Aug 26, 2020

This change worked in my local environment but it is causing a bunch of issues in the static analysis check so I will see if I can figure those out later. If anyone has an idea of what is going on there, I would appreciate the help.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need some tests for this.

As for static analysis check - rebase should help.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right way to go. We will need some tests to verify that this works, and probably some User Guide mention (if it isn't already). I made a couple of logistic notes as well.

* Current request.
*
* @var HTTP\Request|HTTP\IncomingRequest|CLIRequest
* @var HTTP\Request|HTTP\IncomingRequest|CLIRequest|RequestInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should all just be ReqiestInterface. @samsonasik ant compelling reason why the other hints would be needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if method caller use CLIRequest or IncomingRequest only method, that needed, if not, it can only use RequestInterface.

// If a Response instance is returned, the Response will be sent back to the client and script execution will stop
if ($possibleRedirect instanceof ResponseInterface)
{
return $possibleRedirect->send();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, I don't think there is any need to check for RedirectResponse and ResponseInterface since they are handled the same. Let's remove the first block.

paulbalandan and others added 23 commits October 17, 2020 17:42
Co-authored-by: Michal Sniatala <[email protected]>
getSize() does not accept the unit parameter like getSizeByUnit() as stated on the user guide.
Not sure if that is suposed to be like that or it was a "copy/paste" issue on this document, but does induce into error.
@samsonasik
Copy link
Member

@caswell-wc It seems you messed up the rebase, better to make backup branch, back to this branch, reset all, rebase again, and cherry-pick your own commits.

@paulbalandan
Copy link
Member

What's the status of this PR?

@MGatner
Copy link
Member

MGatner commented Nov 17, 2020

@caswell-wc just wrapped up another outstanding PR so hopefully he is looking at this soon. This is possibly resolved by #3864.

@dcaswel
Copy link
Contributor Author

dcaswel commented Nov 18, 2020

This request got pretty messed up and was a really small change so I'm closing it and will recreate it with a fresh fork.

@dcaswel dcaswel closed this Nov 18, 2020
@dcaswel dcaswel deleted the request-from-filters branch November 18, 2020 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.