Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Fixing undefined index 'CLIENT', probably a regression bug #136 #667

Closed
wants to merge 1 commit into from
Closed

Fixing undefined index 'CLIENT', probably a regression bug #136 #667

wants to merge 1 commit into from

Conversation

gnomeontherun
Copy link
Contributor

In the CMS, the 'collection' adapter for the updater fails. This re-implements the fix from earlier.

@joomla-jenkins
Copy link

Unit testing complete. There were 0 failures and 0 errors from 1971 tests and 11142 assertions.
Checkstyle analysis reported 199 warnings and 0 errors.

@pasamio
Copy link
Contributor

pasamio commented Dec 22, 2011

Sounds like the test is broken. JApplicationHelper::getClientInfo() when passed a true for the second param looks up by a string name not a numeric id. My reference example includes a name not an ID because an ID can change between applications however the string identifier should remain consistent:
http://jsitepoint.com/update/modules/list.xml

@gnomeontherun
Copy link
Contributor Author

I'm not sure what you are saying Sam, but you can see this issue if you setup Joomla, login for the first time, and look at the POST request the extension update quickicon makes. It returns errors for no 'client' index, nor is that listed in the docs as an available attribute. http://docs.joomla.org/Deploying_an_Update_Server

Also the test doesn't exist for getClientInfo(), its never been written, and there are no updater tests, but I am guessing that is because it will be moved to the CMS library? Either way, could you help clarify the intent here? If you submit a client_id, do you need a client attribute, or can you submit one or the other?

@pasamio
Copy link
Contributor

pasamio commented Dec 23, 2011

Look at what getClientInfo does and look at what the code seems to think it should be doing. Somewhere something isn't right.

Near as I can tell getClientInfo wants a string in the way it's being called there. Near as I can tell client_id looks like it should be a number. Particularly as the next thing I do is pull out the numeric ID I just got back from getClientInfo and put it into CLIENT_ID...so why did I do that? Seems an elaborate way to replace a variable with the exact same value.

I'm not arguing it's not broken, I'm arguing that the fix doesn't make sense. I said it didn't make sense when it was done the first time and I'm stating it doesn't make sense now.

@gnomeontherun
Copy link
Contributor Author

I didn't understand why that was in there to be honest, I don't get the intent of the code. I was just applying the code from a previous issue, thinking that was the intent.

To me, your example xml doesn't make sense because it uses CLIENT instead of CLIENT_ID. Conventions everywhere else use CLIENT_ID. Your example is the only thing I have to go by, and that doesn't tell me what your original intent for the manifest was, therefore I cannot write a fix. What I don't get is the extensions still are designed to use CLIENT_ID only, so its inconsistent.

This is a problem thats breaking the updater in the CMS though, so this is pretty important.

@pasamio
Copy link
Contributor

pasamio commented Dec 24, 2011

The point of "CLIENT" versus "CLIENT_ID" is that client is a string, and client ID is a number that can change. Someone could map it onto a database table and mix things up. Someone could add a distinct client into Joomla (which is one case I've dealt with, you'll note I submitted a patch to be able to add new clients easily a while back) or potentially have an entirely new app with distinct clients within it. The point of having a name is that if that happens you can still use the update system and it isn't hard coded to ID's assigned by the CMS.

I have no idea why extensions do it, I'd suggest that perhaps it's an oversight that the same mapping code is missing and someone just jammed something in to make it work. I suspect this is again the case here given it really doesn't make sense when you actually look at the code - unless people are putting in the textual description in CLIENT_ID in which case it's logically broken. I also used lists heavily more than individual extension files which would cache the client ID that way. I'll see if over Christmas I can work out what's been borked and fix it.

@pasamio
Copy link
Contributor

pasamio commented Dec 27, 2011

So digging through this, at some point this change was made:
2881906#libraries/joomla/updater/updateadapter.php

Not sure where it came from but what it does is break the bit of code in question. It removes "CLIENT" from the list of update cols which disables the above mapping.

It appears to be related to this broken fix here:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=24338

I suggest a particular tag and some how it turns into breaking something entirely unrelated and thus causing this problem. I probably have this bug notification email sitting somewhere in my inbox unread under the piles of other email.

Now to fix this problem and fix the original problem.

@pasamio
Copy link
Contributor

pasamio commented Dec 27, 2011

Can you check out the following and see if it fixes your problems?
#676

@chdemko
Copy link
Contributor

chdemko commented Jan 2, 2012

Fixed by #676

@chdemko chdemko closed this Jan 2, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants