-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Joomla Update extension compatibility improvements #36531
Joomla Update extension compatibility improvements #36531
Conversation
Improved language string about “Update Information Unavailable” extensions
Make the confirmation checkboxes optional (controlled by component options)
Batch update checks
Co-authored-by: Brian Teeman <brian@teeman.net>
@brianteeman Thanks! I need to create a new editor formatting profile for Joomla PRs in PHPstorm. |
@@ -6,6 +6,8 @@ | |||
COM_JOOMLAUPDATE_CAPTIVE_HEADLINE="Confirm your credentials" | |||
COM_JOOMLAUPDATE_CHECKED_UPDATES="Checked for updates." | |||
COM_JOOMLAUPDATE_CONFIG_CUSTOMURL_LABEL="Custom URL" | |||
COM_JOOMLAUPDATE_CONFIG_NOBACKUPCHECK_LABEL="Disable backup before update confirmation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question?
To me this says the check is displayed but its disabled. When its actually hidden or shown.
Suggestion would be to change the text to
Confirm Backup Before Update
and then change the options to Show/Hide from yes/no
and similar for the extension check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers :) Please take a look at the COM_JOOMLAUPDATE_CONFIG_VERSIONCHECK_LABEL
. That's the one language string I am not sure makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I just re-read the changes you made to the language strings and they dont make sense at all when used with show/hide
Your proposed label is a question and not a description
Maybe be the best option is to move your long text to a description and then have really simple labels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I gave it another go. The labels are very simple and by themselves don't make much sense but with the context provided by the descriptions it should be much easier to understand. With the Show / Hide Help in J4.1 it doesn't matter than one of them is rather long either.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats better
@nikosdion As far as I can see we have to a large part duplicate (or similar) code in the existing fetchExtensionCompatibility method and the new batchextensioncompatibility method . Could that be avoided by putting the common part into some private method which is called by both? Am just asking. If you say "No, let's do that with a future PR" and we maybe have some "Todo" comment not to forget about it, I will survive 😄 . |
@brianteeman I missed that. Thanks! @richard67 I left the old fetchExtensionCompatibility as–is in case there's a third party extension using it. I can't possibly imagine why BUT I also don't see why not. I didn't want to link the two methods in any way as I expect the code in batchExtensionCompatibility to evolve over time whereas the code in fetchExtensionCompatibility should be marked as I'll fix these two and the language inversion of the options later today or tomorrow. I am changing copyright dates on my stuff today. 20 repos down, 15(-ish) to go... |
@nikosdion Makes sense to me. Thanks for the updates. |
A comment on the checkbox nightmare, I personally would remove all "upgrade" confirmations, looking at other software it's done automatically without any user input and that should be the target not to scare people for upgrading. |
@HLeithner I am 100% with you and I have said as much — with much more forceful words — in the year leading to the release of Joomla 4. I had predicted that users would either find this infuriating or it would scare them away from upgrading. As is tradition since 2009, nobody would listen to me until after release when it's too late. Anyway, since I was explicitly told these checkboxes are not going away I will at least make them hideable :) If you can convince everyone else that these checkboxes should I go, I will gleefully remove them without a second thought. I would also not have the pre–update check as a separate step to the update confirmation but as an alternative, i.e. clicking update on the pre–update check page installs the update instead of Joomla blithely asking you “oh, are you sure you want to update, sir?”. Why, yes, didn't I just confirm it?! As I've said before, Joomla Update's reason of existence is to make the update single click. By doing so, it makes people less wary of upgrading which has an overall positive impact on the Joomla ecosystem: more sites are updated, less sites are hacked. This has been Joomla's selling point. Will some updates bork a stark minority of sites (less than 1 in 1000, possibly much closer to 1 in 20,000)? Yes. It happens with all updates of anything: not just FOSS CMS like Joomla and WordPress but products of multi–trillion dollar companies like Apple, Microsoft and Google. Updates to the OS they publish sometimes bork something or even brick devices. It's seldom a bad update. In the majority of cases it's a PEBKAC*/BIOS* issue and there's sod all you can ever do to stop it! () PEBKAC: Problem Exists Between Keyboard and Chair |
@HLeithner @nikosdion the pre-update compatibility checker etc only really makes sense when moving between major versions. I'm sure that was the original intention |
@brianteeman I'm only talking about the backup checkboxes, it's ok to have a text message with "please make a backup before upgrade and be sure it's working" but that should be usually be done by the webhost anyway. I think the upgrade warning for extensions is useful, even in minor versions (in bugfixes releases it's not the fault of the author). But if we need it in the bugfix version because a security breaking change it would make sense to have the option. Minor versions shouldn't break b/c but it's possible that adding new functions could conflict with downstream code. Any way I expressed my opinion, nothing more I want do for 3.x and 4.x branch, for 5.0 this maybe looks different. |
* 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).
@@ -83,8 +83,9 @@ | |||
</div> | |||
</div> | |||
|
|||
<div class="form-check mb-3"> | |||
<input class="form-check-input me-2" type="checkbox" value="" id="joomlaupdate-confirm-backup"> | |||
<div class="form-check mb-3 <?php echo $this->noBackupCheck ? 'd-none' : '' ?>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="form-check mb-3 <?php echo $this->noBackupCheck ? 'd-none' : '' ?>"> | |
<div class="form-check mb-3<?php echo $this->noBackupCheck ? ' d-none' : '' ?>"> |
minor thing but this way you dont have an extra space at the end when there is a backup check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. An extra space is far less important than someone cocking this up 3–5 years down the line because they missed the space before the class name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if they miss the space then they will see it doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My experience is that they will probably miss it because it's an optional feature and class changes are very likely to be done in bulk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ymmv
Add forgotten deprecated tag
@brianteeman @HLeithner I agree that the backup before update checkbox is bullshit. I noticed that there are two kinds of users. Those who understand the risk of updating anything and will take backups anyway and those who stupidly think it's possible to create an update process that never fails (I guess they never had a computer, phone or tablet brick on them on update...). I fixed that by disabling this stupid box by default. Regarding the extensions check, I would agree it makes sense on new major and minor versions (x.y to x+1.y or x.y+1) if and only if it was reliable. It's not. Nowhere even close. But that's something I will keep on working on once this PR is merged. My plan is to first fix orphan extensions, then work on making it clear which extensions do not have update sites, clarify which extensions do not have compatibility information with Joomla x.y and which extensions failed to load information due to a network error (instead of lumping all of them as “we don't know what went wrong” because that's bullshit, of course Joomla knows what happened!). |
Try to make language strings more clear
I have tested this item ✅ successfully on b522560 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36531. |
|
We inverted the language therefore the semantics therefore the default state. The default state is how J4.0 already behaves.
|
@bembelimen Any chance this will be included in Joomla? This is a very much a real issue. I'm getting sick and tired of people contacting me to tell me that my software, which has been perfectly compatible with Joomla 4 since March 2021(!!!) and marked in the update XML files as such, appears to be incompatible with Joomla 4.1 on update from 3.10 and 4.0. Having Joomla display false accusations against third party software and costing 3PDs business is a VERY serious legal matter, not just an annoying bug. We are running out of options here. I have reported this, I have explained it, I have submitted a code fix. What else do you expect me to do? |
Clearly the Joomla project is perfectly fine besmirching the reputation of third party developers who are doing nothing wrong and unwilling to fix its problems. I am sorry I gave you the benefit of the doubt and for trying to help you fix a blatant mistake on your part, one I had warned you FOUR TIMES between 2020 and August 2021. |
@nikosdion Please think it over and restore your branch and reopen this PR. It would be sad to throw away all your work. Let me know if there is something I can do to help you with this decision. |
@richard67 You failed to understand something important. I did not close my PR out of spite. There were two important factors in making that decision:
If you address these two points I am of course willing to reinstate my branch and redo my PR for this issue. I just cannot do it under the current toxic conditions. |
Reopened against 4.2-dev in #37911 |
Summary of Changes
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:
Pinging @zero-24 @HLeithner @wilsonge and also @bembelimen (since this will have to be eventually merged in 4.1).
Testing Instructions
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
andCOM_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:#__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.
PS: I'll come back with another PR to address the “orphan sub–extensions“ issue. I am not including it here because it'd have to be a Database Fix style feature under com_installer which places it out of scope for a PR regarding Joomla Update.
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.