codeigniter4 / CodeIgniter4

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

Docs : set404Override() return status code 200 #6237

Closed ThibautPV closed 2 years ago

ThibautPV commented 2 years ago

PHP Version

7.4

CodeIgniter4 Version

4.2.1

CodeIgniter4 Installation Method

Manual (zip or tar.gz)

Which operating systems have you tested for this bug?

macOS

Which server did you use?

apache

Database

No response

What happened?

If we use the method set404Override() as indicated in the documentation (see here) , it is not specified that the server code returned will be a 200

Steps to Reproduce

Use the code present in the documentation :

$routes->set404Override('App\Errors::show404');

OR

$routes->set404Override(static function () {
    echo view('my_errors/not_found.html');
});

Expected Output

It would be nice to specify that you have to use the method "$response->setStatusCode(404);" (see here) to return the correct error code

An example would certainly be nice for users

Anything else?

No response

ThibautPV commented 2 years ago

Maybe it's not really a bug, but I didn't know to put the "documentation" tag. Sorry.

iRedds commented 2 years ago

I think 404 should return 404 anyway, not 200.

kenjis commented 2 years ago

This behavior comes from v3.

kenjis commented 2 years ago

I think 404 should return 404 anyway, not 200.

It seems counter-intuitive that set404Override() does not return 404 by default. But it comes from v3 and it is a BC if we change it, so we need to discussed.

kenjis commented 2 years ago

@ThibautPV No problem. Only the persons who have the permission can change the labels.

ThibautPV commented 2 years ago

@kenjis @iRedds : I also think it's counter intuitive to return a 200 code, especially for SEO 😉

What does "BC" mean?

iRedds commented 2 years ago

@ThibautPV Breaking Changes

paulbalandan commented 2 years ago

I think 404 should return 404 anyway, not 200.

It seems counter-intuitive that set404Override() does not return 404 by default. But it comes from v3 and it is a BC if we change it, so we need to discussed.

Since this is a bug to begin with, I don't see the fix as much as a BC break. I don't think there are tests in the wild expecting a 200 response from a 404 page. Even if so, that may be minimal.