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

Handling requests sent back from filters #3900

Merged
merged 5 commits into from
Jan 18, 2021

Conversation

dcaswel
Copy link
Contributor

@dcaswel dcaswel commented Nov 18, 2020

Expands the fix of #3085

Description
This is a recreation of #3544 to eliminate issues with the rebase messing up that request.

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 $possibleResponse 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.

Checklist:

  • Securely signed commits
  • N/A Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • N/A User guide updated
  • Conforms to style guide

@dcaswel
Copy link
Contributor Author

dcaswel commented Nov 18, 2020

@MGatner @michalsn I have tested this in my project to make sure it works but I have not found a way to create a PHPUnit test for this. If you have any suggestions on how to create a test for it, I would appreciate it.

@paulbalandan paulbalandan requested a review from MGatner December 1, 2020 06:09
system/CodeIgniter.php Outdated Show resolved Hide resolved
@dcaswel
Copy link
Contributor Author

dcaswel commented Dec 12, 2020

@MGatner I am bumping into PHPStan issues with this that I'm not real sure how to resolve. Could you take a look?

@paulbalandan
Copy link
Member

  590    No error to ignore is reported on line 590.                       
  851    No error to ignore is reported on line 851.                       

There is a comment before those lines @phpstan-ignore-line or @phpstan-ignore-next-line. You just need to remove those comments.

  813    Parameter #2 $request of static method                            
         CodeIgniter\Config\Services::router() expects                     
         CodeIgniter\HTTP\Request|null, CodeIgniter\HTTP\RequestInterface  
         given.                                                            

Just revert your changes to this line and this will go away. Currently you changed:

-        * @var HTTP\Request|HTTP\IncomingRequest|CLIRequest
+	 * @var HTTP\Request|HTTP\IncomingRequest|CLIRequest|RequestInterface
 	 */
 	protected $request;
     Ignored error pattern #Access to an undefined property                     
     CodeIgniter\\HTTP\\Request::\$uri# was not matched in reported errors.     
     Ignored error pattern #Call to an undefined method                         
     CodeIgniter\\HTTP\\Request::(getPath|getSegments|getMethod|setLocale|getP  
     ost)\(\)# was not matched in reported errors.

These two are in the ignoredErrors section of phpstan.neon.dist file. Just delete those two from there.

@dcaswel
Copy link
Contributor Author

dcaswel commented Jan 8, 2021

@paulbalandan @MGatner I made the changes suggested and that seemed to cause new errors. Any ideas?

@MGatner
Copy link
Member

MGatner commented Jan 8, 2021

This is part of the mess of the HTTP layer that I referenced I have been dealing with. I don't think there is a clean fix, but my recommendation is to check if $possibleResponse instance of Request instead and that will ensure the correct type is in place.

The other SA issues I will have to look at not on mobile, but it is possible that PHPStan version has changed and some errors are now detected or not different than before.

system/CodeIgniter.php Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
phpstan.neon.dist Outdated Show resolved Hide resolved
@dcaswel dcaswel force-pushed the request-from-filters branch from 332bfd1 to 8da7f28 Compare January 13, 2021 02:39
@dcaswel dcaswel requested a review from MGatner January 13, 2021 05:25
@MGatner MGatner merged commit 819480a into codeigniter4:develop Jan 18, 2021
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.

3 participants