Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] convert string of a number to a application name #29879

Closed
wants to merge 1 commit into from
Closed

[4.0] convert string of a number to a application name #29879

wants to merge 1 commit into from

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented Jul 1, 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

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jul 1, 2020

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

@PhilETaylor

This comment was marked as abuse.

@nikosdion
Copy link
Contributor

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.

@brianteeman
Copy link
Contributor

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

@nikosdion
Copy link
Contributor

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

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

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

@nikosdion
Copy link
Contributor

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

@nikosdion
Copy link
Contributor

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.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

no point in commenting further

@brianteeman
Copy link
Contributor

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

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor PhilETaylor changed the title convert string of a number to a application name [4.0] convert string of a number to a application name Jul 1, 2020
@PhilETaylor

This comment was marked as abuse.

@nikosdion
Copy link
Contributor

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.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor PhilETaylor closed this Jul 1, 2020
@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@nikosdion
Copy link
Contributor

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.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@nikosdion
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants