User tests: Successful: Unsuccessful:
JInstaller::getParams()
has a code path that results in a null value being returned. This value ultimately gets stored to the extensions table's params column, which can lead to a JSON decoding error.
I don't know a way to reproduce this off hand, however, if you look at where this method is used in the install adapters, you can make sense of how this can be an issue. Sorry, moving quickly that's the best I can give.
None explicitly required however the method still documents itself as returning INI strings (used during 1.5) so I've corrected that and added an inline note indicating the need for a JSON compliant return value.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Category | ⇒ | Libraries |
Without digging deeper into the code to figure out what's going on I can't say I have any other ideas at the moment. I can say though that this patch, whether it's easily testable or not, is indeed a required fix.
What led me here was looking in JInstallerAdapterPlugin
for where the values of the params
field are set. I found a couple of places where it's just hardcoded to {}
and a couple of places where it was using this method. This method has three return spots, the first one being a hardcoded {}
and the last one being the result of json_encode()
(which per https://secure.php.net/manual/en/function.json-encode.php could be a boolean false so maybe the plugin's hitting that?), then there's this one where the return is null, which just does not work with json_decode()
.
All that to say I know this change is right, I just don't know how to validate it.
so @mbabker, aditional info i investigated a little the plugin above
if seems on install it add a query (see install.mysql.utf8.sql inside the package) to add params with valid json (i checked in json lint)
UPDATE #__extensions
SET enabled = '1', params = '{"jQueryVersion":"1.8.2","source":"local","jqunique":"0","jqregex":"([\\\/a-zA-Z0-9_:\\.-]*)jquery([0-9\\.-]|min|pack)*?.js","stripCustom":"0","customScripts":"","stripMootools":"0","stripMootoolsMore":"0","replaceMootools":"0","mootoolsPath":"\/\/ajax.googleapis.com\/ajax\/libs\/mootools\/1.4.5\/mootools-yui-compressed.js","ie6Warning":"1","scrollTop":"1","scrollStyle":"dark","scrollText":"^ Back to Top","lazyLoad":"0","llSelector":"img"}'
WHERE element = 'jblibrary';
So just for testing, after install and hitting the error i done an update query. and now no error
UPDATE <prefix>_extensions SET params = '{}' WHERE element = 'jblibrary';
putting a particular param again i get the error again
UPDATE <prefix>_extensions SET params = '{"jqregex":"([\\\/a-zA-Z0-9_:\\.-]*)jquery([0-9\\.-]|min|pack)*?.js"}' WHERE element = 'jblibrary';
So one of this characters is causing the issue.
So removing the \\
it works no error
UPDATE <prefix>_extensions SET params = '{"jqregex":"([\/a-zA-Z0-9_:.-]*)jquery([0-9.-]|min|pack)*?.js"}' WHERE element = 'jblibrary';
I've also ran into the same issue after updating all my test sites into Joomla 3.6.3. It looks like that new Registry class is throwing an error instead of silently failing when it runs into bad json.
Without looking further, could there be something wrong with installer as it converts \n
inside JSON field to a real newline? Issue is likely the same as with above plugin as PHP has special meaning for \\
as well.
instead of silently failing when it runs into bad json
That's new in the Registry's formatter class. Joomla's API already has too many places where it's silently absorbing errors causing funny behavior if you don't realize it's happening, this was one of them.
Luckily, this one is also easily dealt with in lower level code. See https://github.com/joomla/joomla-cms/blob/3.6.3/libraries/cms/menu/menu.php#L83-L98 as an example. It honestly sucks that Joomla's code both lacks proper error checks in a lot of places and silently discards errors which makes debugging issues more complex.
@mbabker Yes, I agree with you on the part of silently silencing errors. If the error would have been handled properly, the sample data would have been fixed in time and there wouldn't have been some funny behaviour because of missing JSON.
But now I need to track down why the sample data was bad in the first place... I will report back if I find anything weird in Joomla end.
@mahagr if it helps in the case of the other extension was this #12460 (comment)
@andrepereiradasilva Yes, I think my issue is similar to that one; for some reason JSON had newline instead of \n
in it and that newline came originally from mysqldump. I just have no idea why the newline was converted in the first place..
I have tested this item
on code review.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-10-25 23:05:26 |
Closed_By | ⇒ | wilsonge |
Milestone |
Added: |
3.7 == staging?
Robert merged 3.7 to staging and then he started pulling unmerged ones into staging, this is gonna be mess
There aren't going to be anymore 3.6 releases. We just need to move existing pr's from 3.7 to staging and then delete the 3.7 branch. It's not hard...
Thats what @rdeutz did - if i understand correctly (apart from deleting
the branch)
On 26 October 2016 at 08:42, George Wilson notifications@github.com wrote:
There aren't going to be anymore 3.6 releases. We just need to move
existing pr's from 3.7 to staging and then delete the 3.7 branch. It's not
hard...—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#12463 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8QvkgXoOOOWkmzlayaPANg3DfH9Mks5q3wRPgaJpZM4KaQNj
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/
Seems there is a bug in github that will close PRs that started on 3.7 even if you changed the base branch to staging. That's the reason I haven't deleted the 3.7.x branch
i tested installing one of the extensions with issues with this patch already applied and hit the same error (on frontend)
Tested with install from url http://www.joomlabamboo.com/download-document/557-jb-library-joomla-25-/-joomla-30