[#25928] - Allow deep update checking for collection update server
- Closed
- 17 Oct 2020
- Medium
- Build: staging
- # 25928
- Diff
- zero-24:collectionVersionCheck
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #25766
Summary of Changes
This here now implements the option to let joomla do a deep update checking over collection update server results.
As pointed out here #25766 (comment) the collection update server does not support any limiting options like php version or minimum stabilty checks etc.
The reason is that for the initial check only the data of the collection file is used and respected. Only at the actual install time Joomla checks the extension.xml wich than might point to an different version or to no version at all because of the limiting parameters in the extension.xml
This here now allow the extension dev to configure Joomla to do this deep update checking by enable the deep update checking for the collection updateserver.
Testing Instructions
First Test
- install joomla
- install the dummy plugin: https://www.jah-tz.de/downloads/download.php?file=plg_system_dummy.zip&folder=core/extension/dummy/
- try to find an update
- you find the update to 1.0.1
- try to install the update
- it fails as the requirement for this plugin is php 8.0.0
Second Test
- install joomla
apply the update SQL from this patch- apply the patch
- install the dummy plugin: https://www.jah-tz.de/downloads/download.php?file=plg_system_dummy.zip&folder=core/extension/dummy/
- try to find an update
- joomla tells you that it found an update but it requires PHP 8.0.0
Expected result
Actual result
Joomla shows the update but can't install it.
Documentation Changes Required
- The new option for collection update servers need to be documented.
Additional Questions
As of now I have implemented this new option to be a update site wide decision. We could it also change the implementation to just check in the XML for this new option so no need for database changes etc and you just define the parameter in the collection.xml itself and joomla does the deep update checking, This way the site owner could not overwrite this setting. Thougths? Optinions?
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | SQL Administration com_admin Postgresql MS SQL com_joomlaupdate Language & Strings Installation Libraries Front End Plugins |
| Labels |
Added:
?
?
|
||
is this different to the new pre-update check feature
is this different to the new pre-update check feature
Yes as this is primary intended to be used by extension update servers.
- Why do you need a new database field?
- Why are you using camelCase on the field when most of the database schema uses snake_case?
- The current naming implies it is storing a URL when in reality it's some boolean toggle
- Why does this need to be a toggle?
- Until someone writes some proper tests, can we stop adding features to or making changes to the
Joomla\CMS\Updaterclasses because right now it is beyond a pain in the ass to test and validate these things actually work and this critical library has absolutely zero coverage (a task for the testing team if they decide to actually write tests and stop working on architecture or devoting time to things that have nothing to do with testing).
Why do you need a new database field?
As said above it is not an requirement it was just the first idea, can be an option in the XML too.
Why are you using camelCase on the field when most of the database schema uses snake_case?
As it is going to be a option in the XML on your suggestion, this issue can be migrated by the new name.
The current naming implies it is storing a URL when in reality it's some boolean toggle
I can come up with a different name.
Why does this need to be a toggle?
Well for the most update servers this is not relevant so the idea was to make this optional.
Until someone writes some proper tests, can we stop adding features to or making changes to the Joomla\CMS\Updater
Ok, sadly I can't help here for now. :(
ok just double checked the code. With the current architektur of the updater I don't see a way to overcome some sort of database table change as any field not in the database table is dropped here
So we could add an additional field to the #__updates or the #__update_sites table but without bigger changes to the updater i don't see a way to overcome that thing.
The other way around would be to implement the checks just before we add the update to the database but I'm not sure whether this actually works.
| Category | SQL Administration com_admin Postgresql MS SQL com_joomlaupdate Language & Strings Installation Libraries Front End Plugins | ⇒ | Postgresql SQL Installation Libraries |
| Labels |
Removed:
?
|
||
| Category | SQL Postgresql Installation Libraries | ⇒ | Libraries |
Ok have just addressed most of the questions raised here.
I have now removed the sql changes and now use a dedicated setting in the collection.xml (forcedeepextensionchecking="1"). So nothing else is required and it can be set per line in the collection.xml. For sure this is still an optional thing an has to be configured by the extension dev, so no need anymore for an switch in the UI.
The only open point is the Unit test I'm happy to build tests but i have no idea how to do it, so i'm happy for some help me on that. Technical wise this PR here is already working and for sure gets documented by me when the changes here are merged into the core.
I'm happy for feedback on the new / improved implementation
I have tested this item
tested: except the point: apply the update SQL
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25928.
I have tested this item
tested: except the point: apply the update SQL
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25928.
Yes and thanks for your test! @ChristineWk
except the point: apply the update SQL
This is correct as changes to the sql is no longern needed :)
@wilsonge @HLeithner @mbabker As the original reporter did not come back do you guy's think this is a feature we should still implement or should this PR be closed and the feature dropped?
When the feature is not included we would still have the issue with the collection.xml not checking the deeper requirements of an update that could also hit us at some time in the future with the core update as we use the collection updates for our updating. And this feature is also opt-in for the extension dev so we are also b/c for someone relying on the issue we are fixing here.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25928.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25928.
I will postpone to the next release and further discussion.
Ok so better close here @HLeithner ?
| Status | Ready to Commit | ⇒ | Pending |
@HLeithner any update on this one?
| Labels |
Added:
?
|
||
@HLeithner @wilsonge what is you opinion on this is it worth to keep it open for longer or should it just be closed and documented as expected behavior?
This is one of the PRs i have open in a tab to review I mentioned it maintainers chat. Honestly I didn’t understand the term deep update checking either so give me a week to sit down with the code and fully understand what the change is here please and I’ll get back to you
Great, just let me know any question that comes up. In a nutshell with deep update checking we apply the same rules and checks for example the php minimum or stability to updates offered via an collection update server. As of today the update just silently fails as joomla cant get the download url at the actual update step. ;-)
| Category | Libraries | ⇒ | Administration com_admin com_menus com_templates Front End com_newsfeeds com_tags Libraries |
| Labels |
Added:
?
|
||
| Category | Libraries Administration com_admin com_menus com_templates Front End com_newsfeeds com_tags | ⇒ | Libraries |
| Labels |
Removed:
?
|
||
With PHP 7.3.19 there is no plugin update, PHP 8 cannot be enabled yet.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25928.
@adj9 Thanks for your test so you get the message as posted in the inital post right?
It is intended that ("without php 8.0.0") that message comes up.
When it was that case please log your successful test on https://issues.joomla.org
Ah just saw that update server did not server to 3.10 installs as this here was inital build against 3.9 that should be fixed now.
Seems there is still no interest in getting this or similiar PR merged. No need to keep that updated. Closing.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-10-17 09:38:41 |
| Closed_By | ⇒ | zero-24 |
@alex7r please test.