[#33141] - [4] Suppress errors on broken images
- Closed
- 18 Apr 2021
- Medium
- Build: 4.0-dev
- # 33141
- Diff
- PhilETaylor:errorsurpressor
User tests: Successful: Unsuccessful:
Pull Request for Issue #33062
Summary of Changes
After considering all the alternatives, reluctantly use the @ error suppressor to avoid outputting PHP Notices when you have any broken (unparseable) images in your folder.
yes yes yes I know use of the @ error suppressor is highly frowned upon, but there honestly seems to be no alternative here.
Testing Instructions
https://user-images.githubusercontent.com/400092/113942022-17170400-97f8-11eb-88a3-f4164ed1eccc.png
Download this broken image (which was actually generated by Joomla's broken media manager features) and place in your /images folder as broken.png
Set error reporting to maximum in Joomla global config
Visit media manager in the backend.
Actual result BEFORE applying this Pull Request
Ajax call has 2 warnings:
<br/>
<b>Notice</b>
: exif_imagetype(): Error reading from /application/images/broken.png! in
<b>/application/libraries/src/Helper/MediaHelper.php</b>
on line
<b>74</b>
<br/>
<br/>
<b>Notice</b>
: getimagesize(): Error reading from /application/images/broken.png! in
<b>/application/libraries/src/Image/Image.php</b>
on line
<b>185</b>
<br/>
Expected result AFTER applying this Pull Request
Media manager loads correctly.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- 
| Category | ⇒ | Libraries |
| Labels |
Added:
?
|
||
I have tested this item
Before: Media Manager loads a white screen due to the error.
After: Media Manager loads correctly.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33141.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33141.
Not a nice fix maybe, but I'd say for now better than no fix.
Just merge this already - at the end of the day there are some PHP functions that still output notices and warnings instead of throwing exceptions, that's life and we work around it in many places.
Other than using different functions to validate if the image is valid, before calling these methods, there is no way to suppress these errors.
https://www.php.net/manual/en/function.exif-imagetype.php
exif_imagetype() will emit an E_NOTICE and return false if it is unable to read enough bytes from the file to determine the image type.
https://www.php.net/manual/en/function.getimagesize.php
If accessing the filename image is impossible getimagesize() will generate an error of level E_WARNING. On read error, getimagesize() will generate an error of level E_NOTICE.
You did comment it well, so for me it's much better than what we have elsewhere uncommented.
But I'll not merge since I was the one who has set RTC. Always good to have a final instance.
I hate using @ to suppress warnings. Can we be sure that the function exists when called? I mean we get a fatal error with no error if I'm not wrong.
Wouldn't it be better to disable only the "expected" error type (E_NOTICE or E_WARNING) depending on the function call?
Can we be sure that the function exists when called?
/facepalm this has NOTHING to do with if the function exists or not!
I hate using @ to suppress warnings.
We all do. 328 times Joomla 4 does it with 80 of those being in the restore.php used by upgrades. At this least this PR has a reason to use it unlike the amazing overuse of @ in
Wouldn't it be better to disable only the "expected" error type (E_NOTICE or E_WARNING) depending on the function call?
What you are saying is to mess with error_reporting value, before and after making these function calls... so adding up to 5-10 lines of code per function, when the @ suppressor will do all that in a single char.
That would achieve the same thing, in more code, with more complexity.
. .. the amazing overuse of
@in
Maybe we should change the return null; at the end to @return null; ... just to be safe
Maybe something like this which we use for deprecated errors in libraries/bootstrap.php might work for Notice/Warnings - no idea - not tested - looks like a nicerway to handle warnings and errors and would allow us to remove all @ suppression in Joomla 4?
set_error_handler(['Joomla\CMS\Exception\ExceptionHandler', 'handleUserDeprecatedErrors'], E_USER_DEPRECATED);
But of course that is a much larger project to look at separately to this specific issue with the media manager.
Can we be sure that the function exists when called?
/facepalm this has NOTHING to do with if the function exists or not!
maybe you could be less insulting?
The non existing function thing was fixed by php with php 7.0 because fatal errors are exceptions starting with this version. So no problem at least for non existing functions since joomla 4 only support php 7.2.5+
We all do. 328 times Joomla 4 does it with 80 of those being in the restore.php used by upgrades. At this least this PR has a reason to use it unlike the amazing overuse of
@in
Only because we use it doesn't mean it's the right thing to do and I'm not responsible what other people do.
The @ operator is a performance nightmare that's one of the important reason not to use it if possible.
Cour using the Joomla Error handler could be a good way to suppress notices/warnings for broken functions like exif_imagetype and (without checking the code) it should be easy to backtrace the notice to the function call and if it's exif_imagetype for example only write it to the log or show it if debugging is active or something smarter.
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-04-18 17:01:01 |
| Closed_By | ⇒ | PhilETaylor | |
| Labels |
Added:
?
|
||
The non existing function thing was fixed by php with php 7.0 because fatal errors are exceptions starting with this version. So no problem at least for non existing functions since joomla 4 only support php 7.2.5+
You continue to make no sense at all. This PR is nothing to do with "non existing functions" it is about documented notices and warnings generated by calling existing functions.
I think you actually mean this, which is actually from 8.0.0:
Warning: Prior to PHP 8.0.0, it was possible for the @ operator to disable critical errors that will terminate script execution. For example, prepending @ to a call of a function which did not exist, by being unavailable or mistyped, would cause the script to terminate with no indication as to why.
Which, still, has, nothing, to, do, with, this PR!
Only because we use it doesn't mean it's the right thing to do and I'm not responsible what other people do.
Im sorry that you do not believe you are not responsible - but that is literally the job of a maintainer - being responsible for the code other people write that then gets merged.
If you are not responsible then why are you blocking this PR?....
The @ operator is a performance nightmare
But yet we have 328 instances.
be a good way to suppress notices/warnings for broken functions
The functions are not broken. The functions are doing exactly as documented.
The non existing function thing was fixed by php with php 7.0 because fatal errors are exceptions starting with this version. So no problem at least for non existing functions since joomla 4 only support php 7.2.5+
You continue to make no sense at all. This PR is nothing to do with "non existing functions" it is about documented notices and warnings generated by calling existing functions.
I think you actually mean this, which is actually from 8.0.0:
Warning: Prior to PHP 8.0.0, it was possible for the @ operator to disable critical errors that will terminate script execution. For example, prepending @ to a call of a function which did not exist, by being unavailable or mistyped, would cause the script to terminate with no indication as to why.
Which, still, has, nothing, to, do, with, this PR!
Only because we use it doesn't mean it's the right thing to do and I'm not responsible what other people do.
Im sorry that you do not believe you are not responsible - but that is literally the job of a maintainer - being responsible for the code other people write that then gets merged.
If you are not responsible then why are you blocking this PR?....
The @ operator is a performance nightmare
But yet we have 328 instances.
be a good way to suppress notices/warnings for broken functions
The functions are not broken. The functions are doing exactly as documented.
Ok I try it again maybe it's missing in translation...
The non existing function had been suppressed by the @ in versions before PHP 7.0 since 7.0 it's an throwable exception and will not be suppressed except you catch it. What I said with that is that you are right it has nothing to do with this PR:
You make me responsible for code that had been merged by other a long time ago at least I can't remember to merged anything with an @ suppression and no I'm not responsible to rewrite joomla to not use this. No idea why you think that this is my job.
And exactly the 328 instanced should be reconsidered but I except the fact that we don't have the man/women power to do this or at least is not a high priority. But not introducing new once is a start in my opinion.
And no I don't block this PR I just wanted to talk about it, better to solve it with this PR then throwing a notice/warning in production (or development where it maybe unrelated).
The function is broken (even if it's documented) because it throws a notice instead of returning a proper error code or throw an exception or do the same as json_last_error if there is no better way to tell the developer what happens.
I have tested this item✅ successfully on 3740f98
Tested successfully in Beta8-dev of 17 April in Wampserver 3.2.4 using PHp 8.0.2.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33141.