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

Feature/com joomlaupdate improvements #37911

Merged
merged 28 commits into from
Jun 16, 2022
Merged

Feature/com joomlaupdate improvements #37911

merged 28 commits into from
Jun 16, 2022

Conversation

nikosdion
Copy link
Contributor

Summary of Changes

Replaces #36531. Now made against 4.2-dev since it's been nearly five months since I opened the previous one, Joomla 4.1 will be EOL in 2 ½ months and it only makes sense to include this in 4.2 so that updates to 4.3 will not have a problem. Not ideal but, hey, I've reported these issues four times since 2020 (before Joomla 4.0 was released) and done the work to fix them since January 2nd, 2022 (before Joomla 4.1 was released).

This PR addresses a number of issues regarding the way Joomla Update detects and reports the compatibility of installed extensions with the target version of Joomla.

Changes made:

  • Improved language string about “Update Information Unavailable” extensions which does not falsely blame 3PDs for user actions.
  • Make the confirmation checkboxes optional (controlled by component options), allowing advanced users to streamline the update process.
  • Batch update checks to prevent server errors during the pre–update checks.

Pinging @roland-d and @richard67

Testing Instructions

  • Find a cheap shared hosting which enforces request rate limiting and create a site there.
  • Install 20-30 extensions of your choosing.
  • Find any extension that comes in a package.
  • Extract the package.
  • Install all sub–extensions on a site manually.
  • Change the update site to one created for a Joomla 4.2 PR
  • Go to System, Update, Joomla and click on Check for Updates

Actual result BEFORE applying this Pull Request

The Extensions tab shows a lot of Server Error messages.

The extensions listed under “Update Information Unavailable” show a message which blame the developer for not providing an update site.

You have to check the “Acknowledge the warnings about potentially incompatible extensions and proceed with the update.” before clicking on Update. Then you have to check the “I'm prepared for the update and have made a backup.” checkbox to proceed with the update.

Expected result AFTER applying this Pull Request

The Extensions tab no longer shows a lot of Server Error messages.

The extensions listed under “Update Information Unavailable” show a message which merely states that Joomla cannot get compatibility information.

Now go to the Options page for Joomla Update and set “Disable extension version check confirmation” and “Disable backup before update confirmation” to Yes. Click on Save & Close.

You do no longer see the “Acknowledge the warnings about potentially incompatible extensions and proceed with the update.” checkbox; you can simply click on Update. You no longer see the “I'm prepared for the update and have made a backup.” checkbox in the next page. You can just click the button to update.

Documentation Changes Required

The two new options need to be documented.

Language changes required.

Modified language string COM_JOOMLAUPDATE_VIEW_DEFAULT_EXTENSIONS_UPDATE_SERVER_OFFERS_NO_COMPATIBLE_VERSION_NOTES.

Added language strings COM_JOOMLAUPDATE_CONFIG_NOBACKUPCHECK_LABEL and COM_JOOMLAUPDATE_CONFIG_NOVERSIONCHECK_LABEL.

Technical information

I will discuss each change made in detail.

Improved language string about “Update Information Unavailable” extensions

The previous language string asserted that third party developers were to blame for any extensions reported as having no compatibility information. I've already reported this is inaccurate.

The most concerning and most common problem is that a sub–extension belonging to a package may have no package_id in the #__extensions table. This happens in two surprisingly common cases:

  • The user extracted the package's ZIP file and installs the sub–extensions' ZIP files separately. Extraction of the package ZIP happens automatically on macOS using Safari and its default settings.
  • For whatever reason, the site owner used Discover to install sub–extensions whose files are there but they were not fully installed (no #__extensions table record). This happens when people restore backups — manually created, taken through their host or taken through a 3PD extension — haphazardly.

The problem is that even though the 3PD has taken care to provide an update site correctly for the package, the user's actions resulted in a messed up situation. It's not possible for 3PDs to add the package's update site as an update source for each sub–extension, therefore the 3PD has no real way to address this issue. All the while Joomla is lying to the user that the 3PD, the only innocent victim in this case, is to blame.

The new message simply states that Joomla could not determine the update status, without making any false assertions about the root cause.

Note: the user could fix the problem by installing the package type extension. Joomla will then correct the #__extensions table information. However, do remember that a. the user is oblivious to the nature of this problem and b. they likely ended up in this situation because they couldn't install the package extension either due to ignorance / Operating System lying to the user (first case described above) or server limitations (second case). Therefore expecting users to fix a problem using a method they didn't or couldn't use to begin with is... completely pointless?

Make the confirmation checkboxes optional (controlled by component options)

Several users have expressed their extreme displeasure at the two checkboxes leading to a Joomla update on the official Joomla Facebook group and the Joomla Forum. The gist is that they feel it's condescending and a waste of time. They openly admitted they are already ignoring the pre–update checks as a result — something I had already warned you about well over a year ago — and wished for a way to get rid of the checkboxes.

This PR adds two Joomla Update component options to remove each of the checkboxes i.e. the ‘Acknowledge the warnings about potentially incompatible extensions and proceed with the update.’ checkbox in the pre–update check page and the ‘I'm prepared for the update and have made a backup.’ checkbox in the main update page.

These options are disabled by default, i.e. the default state is to have the checkboxes shown and required to protect newbie users from themselves thereby staying true to the reason of existence of these checkboxes.

Batch update checks

Currently, Joomla is trying to run one XHR (AJAX request) per extension ID it needs to determine compatibility information for — and sends these requests all at once, even if there are several dozens of them!

From the server's point of view this looks like a Denial of Service attack. As a result the requests are blocked on most live servers. In some servers the user's IP is temporarily blocked. Joomla does catch that and report a “server error” but this is a cop–out solution which merely undermines the stated objective of the pre–update check feature.

The solution I implemented is fairly straightforward. Joomla Update creates a list of all extension IDs and currently installed versions which need to be checked and sends them to the server. The server will determine the update information of as many extensions as it can within 5 seconds (counted until the beginning of determining an extension's update information). It returns the bulk results and a list of the extensions it didn't have time to check yet. Rinse and repeat until there are no more extensions to check.

This solves the issue because a. there is only one request at a time hitting the server and b. there is enough time between the requests to prevent a server error. Therefore the server no longer thinks it's being hit by a DoS attack.

There is still a situation where this will fail: if an update server is unresponsive in a blocking manner for a long time. In this case all pending extensions will be reported as throwing a server error. In most cases reloading the page resolves the issue.

Further to that, Joomla Update will now convey the server–side warnings, i.e. it will inform the user if a particular update site is inaccessible.

Moreover, in case of a server error, we now call Joomla.ajaxErrorsMessages(xhr) to report the exact error condition experienced instead of only showing everything as errored out.

Nicholas K. Dionysopoulos and others added 11 commits January 1, 2022 17:00
Improved language string about “Update Information Unavailable” extensions
Make the confirmation checkboxes optional (controlled by component options)
* Component options are Show / Hide
* The backup confirmation checkbox is now HIDDEN by default
Hide the backup confirmation checkbox from the upload
and update page (depending on the component option).
Try to make language strings more clear
# Conflicts:
#	administrator/components/com_joomlaupdate/tmpl/upload/default.php
@nikosdion nikosdion requested review from rdeutz and zero-24 as code owners May 27, 2022 15:20
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels May 27, 2022
@Kostelano
Copy link
Contributor

If the option with the checkbox is disabled, then we cannot update, because the button is inactive (relatively recently, the PR was adopted, which allows the button to become available only after the checkbox is filled).

Screenshot_1

@nikosdion
Copy link
Contributor Author

@Kostelano You can try again now. I addressed that change.

@roland-d
Copy link
Contributor

roland-d commented Jun 4, 2022

@nikosdion I was testing this and managed to generate an error on the XHR request and this results in an untranslated string:
image

We should add Text::script('JLIB_JS_AJAX_ERROR_OTHER'); to the preupdatecheck.php file. Can you add that in this PR?

@nikosdion
Copy link
Contributor Author

@roland-d There are actually another four language strings which needs to be pushed as well. They are used by Joomla.Request. Let me see if there's a better way, otherwise I'll modify the file to push all missing strings.

Nicholas K. Dionysopoulos added 2 commits June 5, 2022 10:59
@nikosdion
Copy link
Contributor Author

All right, everywhere else we were indeed pushing the strings from the view template. So I am doing the same in the preupdatecheck.php file.

Nicholas K. Dionysopoulos and others added 2 commits June 16, 2022 09:49
@brianteeman
Copy link
Contributor

brianteeman commented Jun 16, 2022

Find a cheap shared hosting which enforces request rate limiting and create a site there.

Any recommendations on how to simulate this. I tried on my server using mod_reqtimeout but either it doesnt do what I thought or I got the syntax wrong

@brianteeman
Copy link
Contributor

Please remove package-lock from the pr

@nikosdion
Copy link
Contributor Author

@brianteeman It is tricky.

You can emulate with mod_security2, see https://johnleach.co.uk/posts/2012/05/15/rate-limiting-with-apache-and-mod-security/ A rate limit of about 10 would do it on a local server.

You can also use mod_qos with a QS_LocRequestLimit set to something really low, e.g. 3 or 5.

IIRC cPanel uses (or used to use) mod_evasive but this module has not been maintained since 2017.

@brianteeman
Copy link
Contributor

This PR adds two Joomla Update component options to remove each of the checkboxes i.e. the ‘Acknowledge the warnings about potentially incompatible extensions and proceed with the update.’ checkbox in the pre–update check page and the ‘I'm prepared for the update and have made a backup.’ checkbox in the main update page.

These options are disabled by default, i.e. the default state is to have the checkboxes shown and required to protect newbie users from themselves thereby staying true to the reason of existence of these checkboxes.

You have set the default value to 0 (hide) for backupcheck. Is that correct?

@brianteeman
Copy link
Contributor

image

COM_JOOMLAUPDATE_VIEW_DEFAULT_POTENTIALLY_DANGEROUS_PLUGIN_CONFIRM_MESSAGE="Are you sure you want to ignore the warnings about potentially incompatible extensions and proceed with the update?"

COM_JOOMLAUPDATE_VIEW_DEFAULT_NON_CORE_PLUGIN_CONFIRMATION="Acknowledge the warnings about potentially incompatible extensions and proceed with the update."

shouldnt the first one also be changed to use the word acknowledge and not ignore?

@nikosdion
Copy link
Contributor Author

image

COM_JOOMLAUPDATE_VIEW_DEFAULT_POTENTIALLY_DANGEROUS_PLUGIN_CONFIRM_MESSAGE="Are you sure you want to ignore the warnings about potentially incompatible extensions and proceed with the update?"

COM_JOOMLAUPDATE_VIEW_DEFAULT_NON_CORE_PLUGIN_CONFIRMATION="Acknowledge the warnings about potentially incompatible extensions and proceed with the update."

shouldnt the first one also be changed to use the word acknowledge and not ignore?

As you may have noted, I have NOT changed the language strings in this PR. The former was last changed by @bembelimen on 13 July 2021 and the latter also changed by him on 17 October 2021. You can make a separate PR for issues unrelated to this PR.

@nikosdion
Copy link
Contributor Author

This PR adds two Joomla Update component options to remove each of the checkboxes i.e. the ‘Acknowledge the warnings about potentially incompatible extensions and proceed with the update.’ checkbox in the pre–update check page and the ‘I'm prepared for the update and have made a backup.’ checkbox in the main update page.

These options are disabled by default, i.e. the default state is to have the checkboxes shown and required to protect newbie users from themselves thereby staying true to the reason of existence of these checkboxes.

You have set the default value to 0 (hide) for backupcheck. Is that correct?

Good catch, I changed this.

@brianteeman
Copy link
Contributor

<input type="checkbox" class=" me-3" id="noncoreplugins" name="noncoreplugins" value="1" required />

Should be changed to

<input type="checkbox" class="form-check-input me-3" id="noncoreplugins" name="noncoreplugins" value="1" required />

before

image

after

image

@brianteeman
Copy link
Contributor

This PR adds two Joomla Update component options to remove each of the checkboxes i.e. the ‘Acknowledge the warnings about potentially incompatible extensions and proceed with the update.’ checkbox in the pre–update check page and the ‘I'm prepared for the update and have made a backup.’ checkbox in the main update page.

These options are disabled by default, i.e. the default state is to have the checkboxes shown and required to protect newbie users from themselves thereby staying true to the reason of existence of these checkboxes.

You have set the default value to 0 (hide) for backupcheck. Is that correct?

Good catch, I changed this.

Almost. You need to update the default in the php as well.

$this->noBackupCheck = $params->get('backupcheck', 0) == 0;

@nikosdion
Copy link
Contributor Author

<input type="checkbox" class=" me-3" id="noncoreplugins" name="noncoreplugins" value="1" required />

Should be changed to

<input type="checkbox" class="form-check-input me-3" id="noncoreplugins" name="noncoreplugins" value="1" required />

before

image

after

image

Once again, THIS HAS NOTHING TO DO WITH THIS PULL REQUEST. This line was last changed on 13 July 2021 by @bembelimen

Please keep your code review to what I have changed in this Pull Request and only that. Use git blame to find out which line changed when and by whom. I have deliberately kept this PR very specific in scope. It's been extremely hard fixing issues in Joomla! Update even when we have users complaining loudly about it. Therefore I try to fix one small thing at a time, hopefully fixing the problems I have been reporting the year prior to Joomla 4's release within the next 5-10 years (since nobody else can be bothered to do so and my attempts to fix what is blatantly broken are met with way too much resistance)...

@nikosdion
Copy link
Contributor Author

nikosdion commented Jun 16, 2022

Almost. You need to update the default in the php as well.

Done

@brianteeman
Copy link
Contributor

Once again, THIS HAS NOTHING TO DO WITH THIS PULL REQUEST. This line was last changed on 13 July 2021 by @bembelimen

Fine I will create a pr to fix this obvious error

@nikosdion
Copy link
Contributor Author

Once again, THIS HAS NOTHING TO DO WITH THIS PULL REQUEST. This line was last changed on 13 July 2021 by @bembelimen

Fine I will create a pr to fix this obvious error

Thank you! It bugs me too but if I start scope creeping this 6 month old PR will stay not merged for god knows how much longer :p

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on cc2c06b

managed to trick my server into timeout when checking for extension updates. after this pr that didnt happen.

everything else works as described


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

@Kostelano
Copy link
Contributor

I have tested this item ✅ successfully on cc2c06b


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 16, 2022
@roland-d roland-d merged commit c1fdc00 into joomla:4.2-dev Jun 16, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 16, 2022
@roland-d
Copy link
Contributor

Thanks everybody

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants