Skip to content

Conversation

@samsonasik
Copy link
Member

Controller that extends BaseController mostly on http, I think it will be better for IDE detection with mark as @var IncomingRequest for its $request property.

Checklist:

  • Securely signed commits

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

Agreed. My IDE turns red on incomplete interface reliance.

@samsonasik
Copy link
Member Author

@paulbalandan the another solution is probably add & instead of | in the CodeIgniter\Controller's request docblock:

	/**
	 * Instance of the main Request object.
	 *
	 * @var RequestInterface&\CodeIgniter\HTTP\IncomingRequest&\CodeIgniter\HTTP\CLIRequest
	 */
	protected $request;

that will always work, what do you think?

@paulbalandan
Copy link
Member

But I have an experience with phpstan before, using phpstan-strict-rules I think, using the union & in the docblock then passing in a lower type will result in an error. For example here, Property $request expects IncomingRequest, RequestInterface given.

@samsonasik
Copy link
Member Author

samsonasik commented Feb 13, 2021

Yes, PHPStan seems can't verify & for the following:

@var RequestInterface&\CodeIgniter\HTTP\IncomingRequest&\CodeIgniter\HTTP\CLIRequest
------ ---------------------------------------------------------------- 
  44     PHPDoc tag @var for property CodeIgniter\Controller::$request   
         contains unresolvable type. 

The mark as IncomingRequest in BaseController seems the possible solution for now.

@MGatner
Copy link
Member

MGatner commented Feb 13, 2021

This makes me a little uncomfortable with the PSR-7 stuff up in the air, but I think the changes there will require a major version so we could adjust this then anyways. For now I think this will fix a number of other issues for places uses Request and RequestInterface.

@samsonasik
Copy link
Member Author

@MGatner so this PR or the alternative #4282 ?

@MGatner
Copy link
Member

MGatner commented Feb 13, 2021

I don't really have any IDE experience, and you've taught me everything I know about PHPStan. I trust you two to figure this out. 😊

@paulbalandan
Copy link
Member

I'm uncomfortable with a phpstan-ignore-next-line in the phpdoc. Seems like we are suppressing or hiding something. I'd vote for this one instead. And probably we'd do CLIRequest for CommandRunner::$request.

@samsonasik
Copy link
Member Author

@MGatner thank you, I prefer the alternative #4282 but will wait if anyone have another opinion.

@samsonasik
Copy link
Member Author

@paulbalandan Ok, I will merge this 🎉

@samsonasik samsonasik merged commit 363b090 into codeigniter4:develop Feb 13, 2021
@samsonasik samsonasik deleted the add-request-property-var-basecontroller branch February 13, 2021 13:30
@MGatner
Copy link
Member

MGatner commented Feb 13, 2021

Actually, sorry to think about this late, but I think if you run php public/index.php then BaseController::$request will actually be a CLIRequest:

if (is_cli() && ENVIRONMENT !== 'testing')
{
// @codeCoverageIgnoreStart
$this->request = Services::clirequest($this->config);
// @codeCoverageIgnoreEnd
}

We need to test for this and either update the docblock or revert this change until we can figure this out.

@MGatner
Copy link
Member

MGatner commented Feb 13, 2021

function is_cli(): bool
{
return (PHP_SAPI === 'cli' || defined('STDIN'));
}

@MGatner
Copy link
Member

MGatner commented Feb 13, 2021

@paulbalandan @samsonasik Please don't let this one slip by

@samsonasik
Copy link
Member Author

probably @var IncomingRequest&CliRequest is the way to go?

@paulbalandan
Copy link
Member

probably @var IncomingRequest&CliRequest is the way to go?

Hmm. I think this is the way.

@samsonasik
Copy link
Member Author

@MGatner fixed at #4285

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.

4 participants