Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.https://api.drupal.org/api/drupal/core!modules!system!system.module/func...
Currently the function system_retrieve_file generates a variety of Drupal error messages that cannot be suppressed or otherwise modified. This function should probably be modified to return the error to the calling function, allowing that function to determine what happens on an error instead of making an assumption on how errors should be handled.
With the way this is currently developed, it makes using this function for batch imports or other automated systems difficult without clearing all Drupal messages after executing the function. Furthermore, there is no easy way to programmatically determine why this function fails (ie, 404, 403).
Comments
Comment #1
InternetDevels CreditAttribution: InternetDevels commentedlooks like simply...
Comment #4
jamesdixon CreditAttribution: jamesdixon commentedWould it make sense to write these errors to the log using watchdog(), and returning FALSE so we are still getting the same expected return values?
Comment #5
nyirocsaba CreditAttribution: nyirocsaba commentedComment #6
jhedstromComment #7
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch as per the latest Release.
Comment #9
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #11
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedI could apply the patch, so no reroll is needed.Thanks
Comment #13
neetu morwani CreditAttribution: neetu morwani as a volunteer commentedComment #15
neetu morwani CreditAttribution: neetu morwani as a volunteer commentedComment #16
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedThe documentation is also required to be updated if this issue gets committed
https://api.drupal.org/api/drupal/core!modules!system!system.module/func...
Comment #17
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #18
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #26
robpowellI am wondering if it is better to pass the responsibility of catching these exceptions to the implementer and not within the function. As said in the summary of this ticket, currently it uses messenger service which cannot be overriden. Similarly, if you update this to logger, \Drupal::service('logger_factory')->get('foo bar')->error('error message here'), you will have the same issue. I stumbled upon this will running a custom drush command, each run either via messenger/logger I would get the error in the command line. There's nothing to catch in my custom code.
If we pass control to the implementer, ie us, we can catch the result and create workflows we want. There's only two implementations in core so we don't have much risk there but I would like to get some feedback from the community.
Comment #27
Berdir> There's only two implementations in core so we don't have much risk there but I would like to get some feedback from the community.
.. There are also usages in contrib and custom code.
Instead of changing the existing function, similar to how we updated the file_* functions in file.inc, we should move this to a service so it's easier to test and as part of that, change it to (re-)throw exceptions.
Comment #34
kim.pepperI think this can be closed (outdated) as we have deprecated
system_retrieve_filein #3223205: deprecate system_retrieve_file() without replacement