Now that we're PHP5+ we've got exceptions we can finally remove all the variations on drupal_set_message('foo', 'error'); return FALSE; from file.inc and just throw an exception back to the caller and the caller can either act on them or report them to the user.
The exceptions seem to need to end up two places, on the form via form_set_error() and logs via watchdog(). watchdog() wants the untranslated string and arguments and form_set_error() needs a form element to blame so I added a DrupalException class that can handle this bit.
This needs a bit of work still. I haven't updated any of the calling code to test it out I just wanted to get this up here so I could get some feedback.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | use_exceptions_to-348993-16.patch | 3.01 KB | siva_epari |
| #16 | interdiff-13-16.txt | 1.77 KB | siva_epari |
| #13 | use_exceptions_to-348993-13.patch | 1.24 KB | siva_epari |
| #1 | file_348993.patch | 11.22 KB | drewish |
| file_exceptions.patch | 11.11 KB | drewish |
Comments
Comment #1
drewish commentedfixed a couple of obvious bugs.
Comment #2
drewish commentedComment #3
jhedstromThe new field api is also defining it's own Exception class, which directly extends the php Exception class. That should probably be changed to extend the DrupalException class defined in this patch.
Comment #4
kwinters commented#522746: Drupal Exception Wrapper Class is dedicated towards getting better exception handling in core. Once that is fixed, it will just need to be integrated into these modules (if not done already in the other issue).
Comment #5
kwinters commentedComment #6
jhedstromThis is still relevant, I see at least one occurrence of
// @todo Replace drupal_set_message() calls with exceptions instead.Comment #7
jhedstrom'Needs reroll' is a bit of an understatement here.
Comment #8
siva_epari commenteddrupal_set_message replaced as per comment #6
Comment #9
siva_epari commentedSorry. uploaded empty patch file
Comment #10
siva_epari commentedHad added redundant exceptions. Removed it.
Comment #13
siva_epari commentedAdded t() to the exception.
Comment #15
jhedstromI think the failures here are due to these new exceptions not being properly caught.
Comment #16
siva_epari commentedExceptions are caught now. Also fixed a typo in core/modules/system/src/Tests/File/UnmanagedCopyTest.php
s/doesn't exists./doesn't exist./Comment #19
20th commentedI am continuing this task in #2244513: Move the unmanaged file APIs to the file_system service (file.inc).