CodeIgniter icon indicating copy to clipboard operation
CodeIgniter copied to clipboard

Refactor for exceptions

Open jim-parry opened this issue 8 years ago • 3 comments

Enhancement Project: replace usage of show_error() with exceptions. A class or function, instead of calling show_error to display an error message and then exit, should instead throw a PHP exception.

Benefits: improved error handling, better testability.

Related: enhance or replace error-handling writeup in the user guide.

Procedure:

  • In general, replace every show_error() call with a throw new Exception(), or as appropriate, one of the other predefined exceptions.
  • Translations would be mostly dropped in that process as they don't make sense for exceptions ... cases where a translation may be necessary will be discussed/decided on the fly.
  • And just to make the transition smoother, show_error() itself should be altered to throw Exception.

Process:

  • Tackle the affected classes one at a time, making sure that the unit testing and user guide are updated to match.
  • Changes must adhere to the CI style guide.
  • Each converted class, together with its unit testing and documentation changes, should be handled as a separate PR, referencing this issue.

I will create a feature branch for this, so that it doesn't block the main branch.

Affected classes/helpers:

  • system/core/Common (contains the show_error function cited above)
  • system/core/Config
  • system/core/Lang
  • system/core/Loader
  • system/core/Router
  • system/core/Security
  • system/core/URI
  • system/database/DB
  • system/database/DB_Forge
  • system/database/DB_query_builder
  • system/database/DB_utility
  • system/database/drivers/pdo/pdo_driver
  • system/helpers/path_helper
  • system/libraries/Driver
  • system/libraries/Encryption
  • system/libraries/Ftp (also uses lang->line)
  • system/libraries/Migration (also uses lang->line)
  • system/libraries/Pagination
  • system/libraries/Xmlrpc
  • system/libraries/Xmlrpcs

jim-parry avatar Oct 23 '15 08:10 jim-parry

In relation to #4200 ... let's leave show_error() as is for now.

Changing it to just throw an exception may not be as good as I initially thought. It may still happen, but after everything else is done.

narfbg avatar Nov 02 '15 12:11 narfbg

Final: show_error() remains unchanged and a few such calls may remain ... The one triggered by CSRF stays for sure. See #4229.

narfbg avatar Nov 10 '15 18:11 narfbg

@DavidOtis Look above and you'll see 4 pages of mentions that nobody cares about ... Please reference this issue only from your PR descriptions (one reason to actually write a description instead of leaving them empty) and not in every commit message.

narfbg avatar Dec 12 '15 12:12 narfbg