? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
1 Jul 2020

Closes #29778.

Summary of Changes

$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

Testing Instructions

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

Screenshot 2020-06-25 at 16 30 52

After installing Akeeba, to replicate the issue

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

Actual result BEFORE applying this Pull Request

Note JS Console error SyntaxError: JSON Parse error: Unrecognized token '<'

Ajax request with Notices

Screenshot 2020-06-25 at 16 31 42

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

Expected result AFTER applying this Pull Request

No errors.

Documentation Changes Required

None

avatar PhilETaylor PhilETaylor - open - 1 Jul 2020
avatar PhilETaylor PhilETaylor - change - 1 Jul 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jul 2020
Category Libraries
avatar PhilETaylor PhilETaylor - change - 1 Jul 2020
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 Jul 2020
avatar PhilETaylor PhilETaylor - change - 1 Jul 2020
The description was changed
avatar PhilETaylor PhilETaylor - edited - 1 Jul 2020
avatar SharkyKZ
SharkyKZ - comment - 1 Jul 2020

The issue is in your update XML. Client should be the name of the client, not the ID.

avatar nikosdion
nikosdion - comment - 1 Jul 2020

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.

avatar brianteeman
brianteeman - comment - 1 Jul 2020

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

avatar nikosdion
nikosdion - comment - 1 Jul 2020

Thanks @brianteeman. I’m on mobile phone, one handed use, putting kid to sleep. Couldn’t get the link.

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

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)

avatar brianteeman
brianteeman - comment - 1 Jul 2020

@nikosdion it is correct that the code should be an integer it is this pr that it is wrongly changing to a name

avatar nikosdion
nikosdion - comment - 1 Jul 2020

Sorry. Misunderstood. I can’t review (or even view...?) code on GitHub mobile app :/

avatar nikosdion
nikosdion - comment - 1 Jul 2020

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.

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

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

avatar brianteeman
brianteeman - comment - 1 Jul 2020

no point in commenting further

avatar brianteeman
brianteeman - comment - 1 Jul 2020

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

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

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

  1. directly resolving the reported issue
  2. Removing any PHP Notices or Warnings when error reporting is set higher than none.
avatar PhilETaylor PhilETaylor - change - 1 Jul 2020
Title
convert string of a number to a application name
[4.0] convert string of a number to a application name
avatar PhilETaylor PhilETaylor - edited - 1 Jul 2020
avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

Alternative solution proposed - take your pick - both work - both have the same aim, but both are different approaches #29883

avatar nikosdion
nikosdion - comment - 1 Jul 2020

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.

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

I’m sorry. I just tried to help. #close

avatar PhilETaylor PhilETaylor - change - 1 Jul 2020
The description was changed
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-07-01 18:12:29
Closed_By PhilETaylor
Labels Added: ?
avatar PhilETaylor PhilETaylor - close - 1 Jul 2020
avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

@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&amp;type=.xml
https://api.nextendweb.com/v1/?action=joomla_version&platform=joomla&product=smartslider3&pro=0&channel=stable

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

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.

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

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.

avatar PhilETaylor
PhilETaylor - comment - 1 Jul 2020

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 :)

Screenshot 2020-07-01 at 20 34 27

avatar nikosdion
nikosdion - comment - 2 Jul 2020

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 :)

Add a Comment

Login with GitHub to post a comment