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

fix: $request type in BaseController and BaseResource #6910

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 25, 2022

Description
To suppress PHPStan errors like:

Call to an undefined method CodeIgniter\HTTP\RequestInterface::getPost()

Related:

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 kenjis added the 4.3 label Nov 25, 2022
@kenjis
Copy link
Member Author

kenjis commented Nov 25, 2022

I have a question.

What is the RequestInterface in CI4?
Should this eventually have all the public methods of IncommingRequest?
Or should it be the same interface as PSR-7?

@samsonasik
Copy link
Member

samsonasik commented Nov 27, 2022

Typo in PR title : requset -> request

@kenjis kenjis changed the title fix: $requset type in BaseController and BaseResource fix: $request type in BaseController and BaseResource Nov 27, 2022
@kenjis
Copy link
Member Author

kenjis commented Nov 27, 2022

@samsonasik Fixed.
Feel free to fix typos.

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.

I'm not a fan of the assert() part. In BaseController it's not a big problem, but in BaseResource it can be problematic since I can create my own version without extending the "base" class.

As for RequestInterface I believe we wanted it to be compatible with PSR-7. But @MGatner is the person to confirm or deny that.

@MGatner
Copy link
Member

MGatner commented Dec 6, 2022

Yes the intent is to have a fully-compatible PSR-7 set of classes. That said, I have no problem with offering our own framework interfaces for people to swap out. I think ideally these are extensions of the PSR-7 versions and classes are typed to what they actually need (i.e. minimum actual methods required)

@kenjis
Copy link
Member Author

kenjis commented Dec 6, 2022

@MGatner Will RequestInterface have all the IncommingRequest public methods?

@kenjis
Copy link
Member Author

kenjis commented Dec 6, 2022

In BaseController it's not a big problem, but in BaseResource it can be problematic since I can create my own version without extending the "base" class.

The reason I haven't included the assert() in the Controller is because I don't know what the Controller should be.

@michalsn
Copy link
Member

michalsn commented Dec 6, 2022

The reason I haven't included the assert() in the Controller is because I don't know what the Controller should be.

@kenjis Sorry - what I meant was that I can have my own request class like this:

class MyRequest implements RequestInterface

and use it in my services, and it should still work.

In BaseController we can simply change the code if our class not inherit from CLIRequest or IncomingRequest.

But in the BaseResource we will be forced to copy the entire class and use it instead of the original one because assert() will cause an error.

@kenjis
Copy link
Member Author

kenjis commented Dec 6, 2022

Oh, I didn't expect class MyRequest implements RequestInterface.
Because RequestInterface does not have many methods in IncommingRequest.
But it is a correct way to use interfaces.

@kenjis
Copy link
Member Author

kenjis commented Dec 6, 2022

Removed assert() because it is not necessary to suppress PHPStan errors.

@kenjis kenjis requested a review from michalsn December 6, 2022 23:35
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.

Tests are failing, but it's unrelated. Thanks!

To suppress PHPStan error like
Call to an undefined method CodeIgniter\HTTP\RequestInterface::getPost()
It seems assert() is not needed to suppress PHPStan errors.
@kenjis kenjis force-pushed the fix-controller-request-type branch from 08fbc71 to 738ec56 Compare December 7, 2022 07:40
@MGatner
Copy link
Member

MGatner commented Dec 7, 2022

@MGatner Will RequestInterface have all the IncommingRequest public methods?

I hope we won't need to have a RequestInterface, but rather can type everything that needs the framework class to the class itself. Other components that rely on HTTP classes should be typed to the PSR-7 interfaces (when possible), or smaller specific interfaces like LocaleDetection (just an example).

@kenjis kenjis merged commit b395a84 into codeigniter4:4.3 Dec 7, 2022
@kenjis kenjis deleted the fix-controller-request-type branch December 7, 2022 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants