User tests: Successful: Unsuccessful:
Closes #29778.
$this->latest->client
sometimes contains a numeric string (A PHP String containing a number, as opposed to an PHP type integer
) instead of a app name (like 'site', 'administrator' etc)
This pr correctly sends the bool to ApplicationHelper::getClientInfo
depending on what the value of $this->latest->client
is.
if $this->latest->client
= "1" then strlen($this->latest->client) > 1
would be false
, meaning byName
is false.
if $this->latest->client
= "site" then strlen($this->latest->client) > 1
would be true
, meaning byName
is true
see #29778
Hard to test, will only cause error on FIRST LOAD.
Install Akeeba Backup using Install From Url method with url
https://www.akeebabackup.com/download/akeeba-backup/7-2-1/pkg_akeeba-7-2-1-core-zip.zip
After its installed click "Home Dashboard"
Note that in the update checks the Checking extensions
has failed
Joomla 4 admin
Click System
Click Update -> Extensions
Click Clear Cache
Click Home Dashboard
Inspect the Ajax call
Note JS Console error SyntaxError: JSON Parse error: Unrecognized token '<'
Note JS Console error SyntaxError: JSON Parse error: Unrecognized token '<'
Ajax request with Notices
php_1 | 192.168.32.5 - 25/Jun/2020:15:27:10 +0000 "GET /administrator/index.php" 200
php_1 | 192.168.32.5 - 25/Jun/2020:15:27:10 +0000 "GET /administrator/index.php" 200
php_1 | NOTICE: PHP message: PHP Notice: Trying to get property 'id' of non-object in /application/libraries/src/Updater/Adapter/ExtensionAdapter.php on line 333
php_1 | 192.168.32.5 - 25/Jun/2020:15:27:10 +0000 "GET /index.php" 404
php_1 | 192.168.32.5 - 25/Jun/2020:15:27:10 +0000 "GET /index.php" 404
php_1 | NOTICE: PHP message: PHP Notice: Trying to get property 'id' of non-object in /application/libraries/src/Updater/Adapter/ExtensionAdapter.php on line 333
php_1 | 192.168.32.5 - 25/Jun/2020:15:27:10 +0000 "GET /administrator/index.php" 200
webserver_1 | 2020/06/25 15:27:10 [error] 9#9: *155 FastCGI sent in stderr: "PHP message: PHP Notice: Trying to get property 'id' of non-object in /application/libraries/src/Updater/Adapter/ExtensionAdapter.php on line 333PHP message: PHP Notice: Trying to get property 'id' of non-object in /application/libraries/src/Updater/Adapter/ExtensionAdapter.php on line 333" while reading response header from upstream, client: 192.168.32.1, server: , request: "GET /administrator/index.php?option=com_installer&view=update&task=update.ajax&198756102de4392ec83bef34a7d00709=1&cache_timeout=3600&eid=0&skip=212 HTTP/1.1", upstream: "fastcgi://192.168.32.2:9000", host: "127.0.0.1", referrer: "http://127.0.0.1/administrator/index.php"
php_1 | 192.168.32.5 - 25/Jun/2020:15:27:12 +0000 "GET /administrator/index.php" 200
No errors.
None
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
It is documented as an integer in the Joomla documentation. I read the documentation yesterday since i was refactoring my update streams so I know very damn well what it reads.
So, you’re telling me that the documentation is wrong for probably the past 9 years and it is the 3PDs fault for not divining that? LOL.
Fix your crap. The whole update code is a mess and it’s documentation is even worse. Don’t blame 3PDs for following the docs.
This is the link to the documentation that @nikosdion refers to https://docs.joomla.org/Deploying_an_Update_Server where it states it should be an integer
Thanks @brianteeman. I’m on mobile phone, one handed use, putting kid to sleep. Couldn’t get the link.
Well if there are nine years of extensions with numeric values then it might be best to move my changed logic into the getClientInfo method as a catch-all check at the end no info has been resolved from the provided arguments to the method?
Or just change the true to a false on this single changed line and always assume extensions will use numbers and not names (like the documentation states)?
There might also be other places that need review in this case too
(Like Nic, I’m now mobile and can’t check, kids come first)
@nikosdion it is correct that the code should be an integer it is this pr that it is wrongly changing to a name
Sorry. Misunderstood. I can’t review (or even view...?) code on GitHub mobile app :/
FWIW the integer client dates back to 1.6 so it’s 10 years of updates, ie as long as J has been supporting extension updates. Changing this would make it impossible to have J3 and J4 single package extensions. They’d need separate packages with separate update XMLs which is impractical and contrary to J’a stated goal of 3.10 and 4.0 having common 3PD ext packages.
Brian is wrong. The pr is not incorrect
The code before the PR has true for the $byName var - this assumes then that extensions provide a name and not a number
Extension is providing a string of a number
Therefore the pr converts the true to a false if the length of that number is a single char and if longer than a single char passes true as before
no point in commenting further
The code was changed in J4 to remove the deprecated behaviour of using numbers see
https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#Updater
The use of numbers was marked as deprecated and documented here in 2012
https://docs.joomla.org/Design_of_JUpdate#Client_versus_Client_ID
@SharkyKZ is therefore correct
REGARDLESS of what the value in the XML is (and I'm sure @nikosdion and other 3PD will rightly have strong words on the change) Joomla should not be outputting PHP notices and breaking Ajax calls because of it.
This PR resolves THE REPORTED ISSUE by accepting a name or an integer as the input from the XML, correctly calling getClientInfo
with valid arguments depending on the data, and correct returning the correct client identifier from the getClientInfo
method therefore this PR achieves its aims of
Title |
|
Let me put this another way. In 2012 Joomla decided to use the client name instead of its numeric ID. Yet, the documentation was updated to read this:
client – Required for modules and templates as of 3.2.0. – The client ID of the extension, which can be found by looking inside the #_extensions table. To date, use 0 for "site" and 1 for "administrator". Warning! Plugins and front-end modules are automatically installed with a client of 0 (site), but you will need to specify the client in an update or it will default to 1 (administrator) and then found update would not be shown because it would not match any extension. Components are automatically installed with a client of 1, which is currently the default.
Warning: The tag name is for Joomla! 2.5 and <client_id> for 1.6 and 1.7. If you use <client_id> (rather than ) on a 2.5 site, it will be ignored.
Of course I read the code back in Joomla 3.1 – when I contributed the extra_query code – and I reasonably assumed that if the documentation says it's an integer and the code says it can either be an integer or a string the truth MUST be that the integer is the new way and the string is the old way. Way to go Joomla. Extra kudos for blaming the 3PDs for following your official but incorrect documentation. That inspires a lot of confidence.
@PhilETaylor I agree that Joomla shouldn't throw PHP notices, warnings or errors if the input is not what it expects. However, at this point, I wouldn't be able to sincerely tell you what is the EXPECTED input since the code says one thing, the documentation says another. The correct way to handle input is obviously to spit it against the wind and hope it doesn't land back on your face.
I’m sorry. I just tried to help. #close
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-07-01 18:12:29 |
Closed_By | ⇒ | PhilETaylor | |
Labels |
Added:
?
|
@nikosdion I feel your pain. Even the docblock for the method called was factually incorrect /facepalm
I have checked some of the extensions listed as top rated on the JED (well the ones I could get for free, most are paid for) and a few more...
16 sites
50% used 0/1
50% used site/administrator
As has been made clear, my approach to resolving the PHP issues that will occur with approx 50% of extensions is not acceptable, I'll repopen the issue #29778 and let someone else have a go.
Is Joomla really going to throw all 3pd under a bus for one line of code?
I have manually been through the following.
0
https://virtuext.com/vx-orphanimages.xml
https://cdn.joomlacontenteditor.net/updates/xml/editor/pkg_jce.xml
http://virtuemart.net/releases/vm3/mod_virtuemart_manufacturer_update.xml
https://www.joomlashine.com/versioning/extensions/sunfwframework.xml
http://update.joomlart.com/service/tracking/j16/plg_jabookmark.xml
http://update.arkextensions.com/arkeditor.xml
1
https://update.joomlack.fr/mobilemenuck_light_update.xml
https://update.joomlapolis.net/versions/pkg_communitybuilder.xml
site
https://check.kubik-rubik.de/updates/lazyloadforjoomla.xml
http://updates.favthemes.com/extensions/favtestimonials.xml
https://joomla-monster.com/updates/mod_jm_testimonials.xml
https://www.jevents.net/updates/pkg_jevents-update.xml
http://www.eshiol.it/updates/packages/j2xml-updates?format=xml&dummy=extension.xml
https://www.faboba.com/index.php?option=com_ars&view=update&task=stream&format=xml&id=2&lang=fr
administrator
https://download.regularlabs.com/updates.xml?e=advancedmodulemanager&type=.xml
https://api.nextendweb.com/v1/?action=joomla_version&platform=joomla&product=smartslider3&pro=0&channel=stable
Lastly, after this whole conversation, the IRONY that is the fact that the line of code I changed, is designed to be taking a string $id
and converting it back to an client id of type integer
, after 8 years of "deprecated use of integers for client, use strings instead"... just makes me laugh... the irony is painful.
Might be worth bookmarking https://github.com/joomla/schemas as well - no idea if that will be kept up to date (even the link in the read me is a 404!) but currently that has clientType as strings
Sent from my iPhone
On 1 Jul 2020, at 20:21, Nicholas K. Dionysopoulos notifications@github.com wrote:
Since I see that Faboba is using ARS but his update stream uses strings. I faintly remembered that at some point I was using strings and I was wondering how old an ARS release he's using. So I searched the ARS repo to figure out what happened.There was no client ID at all before October 31st, 2011 when Joomla 1.7's bug about components without a client ID was brought to my attention. It continued being an integer until March 28th, 2012 when I started using the strings site and administrator when someone else pointed out that Joomla 2.5 is no longer using the client_id tag and the client is no longer an integer in Joomla 2.5.
These two changes were made "blind" in the sense that I was not using the Joomla extensions updater myself. At that point it didn't support paid extensions. I only started using it after Joomla 3.2 was released in November 2013. My first versions to use Joomla update streams were released between December 2013 and February 2014 – I checked my release notes.
Anyway, three years after the previous change, on March 20th, 2015, I changed it back to integers because another person complained the string values were Joomla 2.5 specific and were now deprecated in favour of integers. I consulted the documentation and Joomla 3.4's code which seemed to confirm the finding. Since I had removed Joomla! 2.5 support a month prior and the official documentation agreed with the person filing the bug report I made the change.
As to the core code, the updates table is using an integer for the client for efficiency reasons. So, yeah, it needs to convert whatever you give it to an integer. Based on the URL Brian found and posted earlier the idea behind using a string in the XML file is that the integer client ID can change between Joomla versions but the client name would remain stable. Of course we all know that historically the opposite has been true (admin vs administrator, anyone?) and the integer client ID is deeply embedded in the core and has never changed since, dunno... Before 1.6 is my best guess. At this point it's been way too long to even remember. So the whole string vs integer change was useless, miscommunicated and a total clusterfsck – not to put too fine a point on it. Where is my "hitting head against desk" emoji when I need it?
Anyway, back I go to strings and hope nothing will break yet again. I'm getting too old for this crap.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or unsubscribe.
is that the integer client ID can change between Joomla versions but the client name would remain stable
LOL
9 years ago they were set in stone, hard coded, in
and never changed again :)
The ID predates your finding. Joomla 1.5's application object had an isSite method which checked for ID 0. That would be ~13 years ago.
As for the schemas, I did have a faint recollection of having seen that somewhere so I did look for them last night but I couldn't find anything. I didn't think to go through the bazillion Joomla.org repositories here on GitHub. Thank you for doing this kind of software archaeology :)
The issue is in your update XML. Client should be the name of the client, not the ID.