Bug: Custom 404 error handler. Response caching.

[iRedds]

PHP Version

7.3

CodeIgniter4 Version

4.1.8

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

No response

What happened?

If the custom 404 error handler does not output data to the buffer, then an empty body is returned.

Steps to Reproduce

$routes->set404Override('App\Controllers\Home::err');

public function err()
{
    return \Config\Services::response()->setJSON([1, 2, 3]);
}

Expected Output

[
    1, 
    2,
    3,
]

Anything else?

In the CodeIgniter::display404errors() handler, the result of the CodeIgniter::runController() method is not passed to the CodeIgniter::gatherOutput() method.

CodeIgniter4/system/CodeIgniter.php

Lines 859 to 867 in 827c360

	$controller = $this->createController();
	$this->runController($controller);
	}
	unset($override);
	$cacheConfig = new Cache();
	$this->gatherOutput($cacheConfig);
	$this->sendResponse();

CodeIgniter::gatherOutput() If the data is not passed and there was no output to the buffer, then the response body is reset to an empty string.

$this->output = ob_get_contents(); // empty string
$this->response->setBody($this->output); // reset

I think that we need to consider not only what the CodeIgniter::runController() method returns, but also what is contained in the response body.
That is, it is necessary to define a priority between echo, return and Response::setBody().

[kenjis]

That is, it is necessary to define a priority between echo, return and Response::setBody().

The current gatherOutput() is:

• echoed output + (returned response's body or returned string)

It seems no problem.

[iRedds]

I don't think this is the correct approach. Although I may be wrong.

CodeIgniter4/system/CodeIgniter.php

Lines 926 to 928 in 93b8bca

	if (is_string($returned)) {
	$this->output .= $returned;
	}

[kenjis]

It may not be correct, but what's wrong?

[iRedds]

I mean the concatenation of the data from the buffer and the returned string.
It seems to me that it is better to use "protection against the fool". And in the future, refuse to capture the buffer.

[kenjis]

Do you think the following is better?

• echoed output
• or returned response's body
• or returned string

I don't understand why it is "protection against the fool".

[iRedds]

For the same reason why you write a return type.
This makes the code smaller. This simplifies the code.
I would prefer just return ResponseInterface =)

[lonnieezell]

I think it's dangerous to change that. I don't recall the reasoning it was there exactly but if it would be changed it would need a good chunk of additional tests to ensure all of the varieties of responses are covered, no matter how crazy they might be, so we don't break a less experienced coder's mess.