codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.4k stars 1.9k forks source link

Bug: sendResponse doesn't respect returnResponse parameter in codeigniter.php #6650

Closed AmitSonkhiya closed 2 years ago

AmitSonkhiya commented 2 years ago

PHP Version

8.1

CodeIgniter4 Version

4.2.7

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

cli

Database

MariaDB 10.2

What happened?

File in the question: system/CodeIgniter.php

The run() method (line: 292) in the file has the second parameter to return the response instead of printing it. However, in the case of page not found exception, the script straight prints the result. Which makes it harder to cache the exception response in the caller script.

Indeed there are other ways to deal with the issue but all of them require extending a few methods in the child class (extending Codeigniter.php) just to include a return statement.

Steps to Reproduce

  1. Use a non-traditional server like ReactPHP or Swoole.
  2. Initiate requests from the server to CodeIgniter.
    $app = Services::codeigniter(config('App'));
    $app->initialize();
    $app->setContext('php-cli');
  3. Execute the run() method along with $returnResponse set to true. $app->run(null, true);
  4. Override default 404 page with a controller/method.
  5. Request a resource that doesn't exist.
  6. In the case of a response code 200, CI will return the HTTP\Response object.
  7. In the case of a response code 404, CI will return null and the result (HTML formatted response) will be written to the server's log file.

I haven't tested but the behavior might be seen in the case of response code 500 as well.

Expected Output

When there is a Page not found exception, the server should return the response object instead of printing (echoing) it. Same is true when a internal server error occurs.

Anything else?

I have extended the core class to overcome this issue but I strongly believe that the framework should respect the type of return requested for either of the HTTP response codes.

Hence it, would be better to make $returnResponse as a class property and change the sendResponse() method's behavior.

Otherwise below are lines to update in the system/CodeIgniter.php file to let the the application return the response when it is asked rather than echoing when a page not found or server error exceptions occur.

Line 312 in the run() method return $this->sendResponse($returnResponse);

Line 348 in the run() method return $this->sendResponse($returnResponse);

Line 354 pass the parameter $this->display404errors($e, $returnResponse);

Line 914 method signature protected function display404errors(PageNotFoundException $e, bool $returnResponse = false)

Line 937 in the display404errors() method return $this->sendResponse($returnResponse);

Line 1072 sendResponse() method

    protected function sendResponse($returnResponse = false)
    {
        if (!$returnResponse) {
            $this->response->pretend($this->useSafeOutput)->send();
            return;
        }
        return $this->response;
    }
kenjis commented 2 years ago

Thank you for reporting. Why don't you send a PR? See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md

AmitSonkhiya commented 2 years ago

I didn't test the changes yet when a server error occurs or there is a redirection exception. Hence, I don't think it would be right to send the PR at the moment without following all specifications for the same. Thanks

kenjis commented 2 years ago

I see.

By the way, do you make it to run CI4 on ReactPHP or Swoole?

AmitSonkhiya commented 2 years ago

Not fully yet. It is Open Swoole.

I'm still exercising to populate the HTTP/IncomingRequest object from the server's request object.

kenjis commented 2 years ago

I would recommend you see https://github.com/monkenWu/CodeIgniter4-Burner

I would like to support Long-living PHP like Swoole, but it looks like a long long way.

monkenWu commented 2 years ago

Hi, I'm glad @kenjis mentioned my Library.

I had a lot of problems, as you did, not only with the Request and Response object conversion, but after that you will have problems with the Debug bar, SESSION and file upload; The point of view from the long-living PHP applications, the CodeIgniter4 is not well designed. This is because CodeIgniter4 is not designed to stay in memory.

I have been spending my spare time researching how to support long-living PHP applications without changing the CodeIgniter4 source code. I've had some good results with CodeIgniter4-Burner so far, so feel free to discuss it with me.

AmitSonkhiya commented 2 years ago

Thanks @kenjis for mentioning the library and the contributors to giving the faster frameworks. I don't think it would be wise to carry discussion over a bug ticket hence I have further asked from monkenWu on the linked issue.

There is a feedback for CI team as well.

kenjis commented 2 years ago

@monkenWu I really appreciate your great work. I have not tried Burner yet, but I would like to look into it soon.

I have started a thread in the Forum to exchange information. https://forum.codeigniter.com/showthread.php?tid=83970

MGatner commented 2 years ago

@AmitSonkhiya Could you please try out @kenjis PR linked above and see if it fixes this issue for you?

AmitSonkhiya commented 2 years ago

@MGatner I would do. Just give me 3 days to get back at work. Thanks for the update.

AmitSonkhiya commented 2 years ago

@MGatner @kenjis

Thanks for the update. This bug doesn't appear anymore using CI from the development branch.

MGatner commented 2 years ago

Great! Thanks for confirming @AmitSonkhiya