[#38907] - Ignore PHP warnings raised by simplexml_load_file()
- Fixed in Code Base
- 15 Oct 2022
- Medium
- Build: 4.2-dev
- # 38907
- Diff
- BrainforgeUK:patch-10
User tests: Successful: Unsuccessful:
Such warnings cause com_installer to raise a JSON error because it messes up the Ajax response.
Pull Request for Issue # .
Summary of Changes
Supress such warnings by adding @ to simplexml_load_file($file)
Testing Instructions
Install an extension with duplicate client attributes
<extension type="module" client="administrator" method="upgrade" version="3.1" client="administrator" position="cpanel">
Installs OK.
Try to install any other extension and get JSON error.
For some reason the installer runs through all the existing manifest files and hits the problem when it meets the previously installed extension.
Actual result BEFORE applying this Pull Request
Get JSON parse error
Expected result AFTER applying this Pull Request
Not a problem with extension being installed so would not expect an error.
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
In general I also would prefer libxml_use_internal_errors, but for this particular case it is fine with @, I think.
Because we just ignore broken XML here.
Why should we do here an exception? When a manifest file can't be loaded, the developer needs to know why. Otherwise it would be very frustrating, when the extension namespace is not loaded and no error is shown.
The trouble with this error is that it is with a previously installed extension not the extension being installed.
Causes confusion as the developer on the extension being installed gets the blame!
Why should we do here an exception?
joomla-cms/libraries/namespacemap.php
Lines 233 to 236 in e9dfc25
The PR is fine for what it is.
this should not be silently ignored
It already designed to be silently ignored.
If you want to change this then it is totally different issue.
To achieve what you asked someone should review whole namespacemap.php thing.
And we probably cannot change it in 4x.
All I'm asking for is to properly catch the error how it is designed in PHP land and not to hide it. I was not talking about any refactoring at all.
For me it is good how it done, it can be improved later.
@laoneo it's a bit unfair to request to fix this if you did more or less the same 5 month ago #37595
anyway never use @ please use the 3rd parameter LIBXML_NOWARNING for now.
$xml = simplexml_load_file($file, 'SimpleXMLElement', LIBXML_NOWARNING);The complete function doesn't expect any error which is really strange why we are so lazy here... maybe it's too critical to create the namespace map compared to the problem when a extension is broken? Any way we need to report error somewhere maybe in the JLOG?
As I suspected this PR raises more questions - which should be dealt with elsewhere.
Why was the error in the first extension not spotted and the installation failed?
Instead the error does not occur until another extension, from another developer, is installed.
The 2nd developer gets blamed and not the 1st developer!
This fix solves the immediate problem.
Anyway using LIBXML_NOWARNING sounds like a good idea.
Will implement shortly.
| Labels |
Added:
?
?
|
||
@BrainforgeUK can you please check whether this work:
$xml = simplexml_load_file($file, 'SimpleXMLElement', LIBXML_NOERROR | LIBXML_ERR_NONE);Cannot currently check on my own. Duplicated attribute is an Error in XML, not Warning.
No need for LIBXML_ERR_NONE, on its own it did not help.
LIBXML_NOERROR achieves the objective of avoiding the JSON error.
I have tested this item
With LIBXML_NOERROR also works
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38907.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38907.
| Status | Pending | ⇒ | Ready to Commit |
r2c
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38907.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-10-15 08:42:52 |
| Closed_By | ⇒ | HLeithner | |
| Labels |
Added:
?
|
||
thanks
I have tested this item✅ successfully on e9dfc25
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38907.