bcit-ci / CodeIgniter

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

Exceptions.php - please remove \t (in cli) and <p> (in html) #6077

Closed bunglegrind closed 2 years ago

bunglegrind commented 2 years ago

In Exceptions.php:

https://github.com/bcit-ci/CodeIgniter/blob/16ab872de21dcf938d9c6243d732b0206c8efa7f/system/core/Exceptions.php#L171

https://github.com/bcit-ci/CodeIgniter/blob/16ab872de21dcf938d9c6243d732b0206c8efa7f/system/core/Exceptions.php#L177

A spacing char (\t or < p >) is inserted in the error message when show_error is invoked. IMHO this is the wrong place: they should be inserted in errors_general.php since they are related to the formatting of the error message. Indeed, they may create presentation issues in a custom error template.

gxgpet commented 2 years ago

Well, the messages' composition is coupled with their views in application/views/errors/, indeed. But by now, existing users may have overcome this already by ensuring they treat the $message formatted with \t and <p> elements on their custom error templates. Changing this will create a BC break.

narfbg commented 2 years ago

I agree, in principle, that this should ideally be handled in the templates themselves. However, they are just that - templates, with zero logic inside them - and only expect a single string variable. Such a change would require all templates to start checking whether $message is a string or an array and apply appropriate logic - unnecessary and annoying work that would make the templates harder to work with in the future, even if there was no BC break.

If you have a specific problem that you can explain better, please submit a pull request with the desired changes, but this is not worth it for the sake of principle alone.

bunglegrind commented 2 years ago

I agree, in principle, that this should ideally be handled in the templates themselves. However, they are just that - templates, with zero logic inside them - and only expect a single string variable. Such a change would require all templates to start checking whether $message is a string or an array and apply appropriate logic - unnecessary and annoying work that would make the templates harder to work with in the future, even if there was no BC break.

Well, in that case a helper function should convert the array in a string. Anyway, I can't see any solution without breaking existing code.