Use Exceptions on CommonDBTM CRUD flow

[trasher]

Just a Quick'N'Dirty try for now.

This has been opened as PoC to begin the discussion; the base idea is to prevent silent fails of strange issues when a false is sometimes returned (and often not checked).

I've quickly discussed about that with Cédric, and he tells me it will throw an error instead of redirecting the user to a page with a comprehensible error message. This means I'll have to handle those Exceptions in controllers (OK, no problem) but also in all front files (that's not an interesting job, but I'm OK to do that for the need).

Of course, this PR still need work to be complete before making any change on front files (it will be done last); but I'd like to know if anyone has remarks about it before doing anything else.

[cconard96]

From a frontend/API standpoint IMO any error system for CRUD actions should allow specifying per-field issues, even if only a global error message is used to start.

[cconard96]

Suggested change

	throw new CloneException('Cannot clone item on a DB slave.');
	throw new CloneException('Cannot clone item on a DB replica.');

Not sure this kind of error should be shown to users as they won't know what it means. Having a user-facing message separate from the internal exception message could be useful, similar to what I did for API exceptions in /Glpi/Api/HL/APIException.

[trasher]

Exception messages are not meant to be displayed to the user; they're just for logs.
The idea is to properly catch them and display a common comprehensible message (in this case, just something like "Item has not been cloned, an error occurred").

[cedric-anne]

This could indeed permit to block backend processes when an operation fails, rather than continuing the operation.

To detect places where a try/catch has to e added, you could probably change the return type of the methods to void and expect PHPStan to detect all invalid if ($item->add(...)) calls (Result of method {xxx} (void) is used errors will be shown).

IMHO, such replacement may also be automatized with a custom rector extension. This would help a lots if it can be achieved since it would permit to easilly do these replacements in our plugins.

For the exception themselves, I cannot tell if we need to add more precise exceptions PrepareAddInputException, UnicityException, AddHookException, ...

[trasher]

This could indeed permit to block backend processes when an operation fails, rather than continuing the operation.

To detect places where a try/catch has to e added, you could probably change the return type of the methods to void and expect PHPStan to detect all invalid if ($item->add(...)) calls (Result of method {xxx} (void) is used errors will be shown).

That can be an idea, I'll try.

IMHO, such replacement may also be automatized with a custom rector extension. This would help a lots if it can be achieved since it would permit to easilly do these replacements in our plugins.

Well, maybe yes, but I'm not at all familiar with rector; I have no idea what would be the complexity of such an extension.

For the exception themselves, I cannot tell if we need to add more precise exceptions PrepareAddInputException, UnicityException, AddHookException, ...

Maybe could we concentrate in a first time on direct CRUD errors. On the current iteration, prepare* are still returning false; it can be changed in a second iteration (or in this one, but later).