? Pending

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
18 Oct 2016

Summary of Changes

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.

Testing Instructions

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.

Documentation Changes Required

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.

avatar mbabker mbabker - open - 18 Oct 2016
avatar mbabker mbabker - change - 18 Oct 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Oct 2016
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 18 Oct 2016
Category Libraries
avatar andrepereiradasilva
andrepereiradasilva - comment - 19 Oct 2016

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

avatar mbabker
mbabker - comment - 19 Oct 2016

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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 19 Oct 2016

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';
avatar mahagr
mahagr - comment - 19 Oct 2016

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.

avatar mbabker
mbabker - comment - 19 Oct 2016

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.

avatar mahagr
mahagr - comment - 20 Oct 2016

@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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 20 Oct 2016

@mahagr if it helps in the case of the other extension was this #12460 (comment)

avatar mahagr
mahagr - comment - 20 Oct 2016

@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..

avatar andrepereiradasilva andrepereiradasilva - test_item - 20 Oct 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 20 Oct 2016

I have tested this item successfully on f6eef99

on code review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12463.

avatar wilsonge wilsonge - reference | 997518f - 25 Oct 16
avatar wilsonge wilsonge - merge - 25 Oct 2016
avatar wilsonge wilsonge - close - 25 Oct 2016
avatar wilsonge wilsonge - change - 25 Oct 2016
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-10-25 23:05:26
Closed_By wilsonge
avatar wilsonge wilsonge - close - 25 Oct 2016
avatar wilsonge wilsonge - merge - 25 Oct 2016
avatar wilsonge wilsonge - change - 25 Oct 2016
Milestone Added:
avatar mbabker mbabker - head_ref_deleted - 25 Oct 2016
avatar dgt41
dgt41 - comment - 25 Oct 2016

@wilsonge you've merged this in 3.7 not in staging

avatar wilsonge
wilsonge - comment - 26 Oct 2016

3.7 == staging?

avatar dgt41
dgt41 - comment - 26 Oct 2016

Robert merged 3.7 to staging and then he started pulling unmerged ones into staging, this is gonna be mess

avatar wilsonge
wilsonge - comment - 26 Oct 2016

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...

avatar brianteeman
brianteeman - comment - 26 Oct 2016

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/

avatar rdeutz
rdeutz - comment - 26 Oct 2016

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

Add a Comment

Login with GitHub to post a comment