User tests: Successful: Unsuccessful:
Pull Request to distinguish 3rd party extensions that do NOT support the Joomla Extension Update
The JED lists for all extensions if they support the Joomla Extension Update.
However, you cannot see if the extensions you've installed in your Joomla site support the Joomla Extension Update. Maybe the JED lists some extensions that they support the Update, but you are using older versions in your site that do not support it.
To make it easier to see which extensions support the Joomla Extension Update, I've added a warning for 3rd party extensions that do not support it.
Install a 3rd party extension that uses the Joomla Update, for example
https://db8.eu/download/component/db8-optimize-site
Install this patch.
Go to Extensions: Manage. You should see the installed 3rd party extension.
Go to Extensions: Update Sites. Remove the reference for the installed 3rd party extension.
Go to Extensions: Manage. You should see the installed 3rd party extension plus a message that it does not support Joomla update
The extension did support the Joomla Update, but the reference is missing in the Update Sites and not checked anymore. To enable it again:
Go to Extensions: Update Sites. Choose Rebuild.
Go to Extensions: Manage. You should see the installed 3rd party extension without the warning message.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_installer Language & Strings |
Labels |
Added:
?
?
|
@wilsonge What is correct now to check for core extensions? Item ID 999 like done in this PR here? Or 9999 like done in PR #16494 ?
I ask you because the other PR seems to be based on your input.
@pe7er Maybe you should change the 999 to 9999 for check of the extension id, see #16494 (comment).
The sidenote in the comment I mentioned above makes me think it is not really save to use the ID number to check if an extension is core or not. But what else way do we have beside a hard-coded list which would have to be maintained every time an extension is added (and so would be a shit)?
Regarding the question on how to detect if an extension is core or not which I mentioned in my comment above (999 or 9999), this PR here should not wait for a solution.
Let it continue to use the 999.
Any later PR implementing a safe check will also have to look for such things and replace them by using the new way which it implements.
But this PR needs 2 changes to be good:
This sounds a really nice feature, couple of very minor comments on the "Update Sites not unavailable" text....
These are very minor comments, and no problem at all if folk disagree.
Thanks :)
Does nobody here notice that "not unavailable" is a mistake and it should either be "unavailable" or "not available"??? Do you sleep while reading?
I think @richard67 that you're probably right, may have been sleep reading there.
I'd say, on reflection, that double negatives tend to be confusing, so you're right, "unavailable" or "not available" would be better wording.
Also, wording aside "not unavailable" is, I guess not the intended description.
@AndyGaskell Well, at the beginning the text was "unavailable", but to me "not available" sounded better so I suggested that, and @pe7er wanted to follow my suggestion but maybe missunderstood me or understood me right but made a mistake, and so he changed it to "not unavailable", but this negates the original meaning and so displays wrong information.
@richard67 You can submit a PR to his branch to be merged.
I know, if he wants I can do that later.
I will have to do that anyway if my PR with the new extensions helper class will be merged before this one.
@pe7er What remains to be done in my point of view is what @AlexRed wrote in his comment above:
If the extensions belong to a package, which means the "Package ID" column is not empty, the status of the package's update site has to be shown and not the one of the extension. This can be done by modifying the SQL select statement in file administrator/components/com_installer/models/manage.php by joining the update_sites_extensions table a second time, this time to the package_id column, and then checking the update_site_id of the package if it is set, otherwise the one of the extension. I am working on a PR to your repo for that.
@pe7er Another thing is the check to exclude core extensions. I think it is not necessary to exclude all core extensions because the only core extensions which have update sites are those which can be excluded like it is done here: https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/models/updatesites.php#L382.
All other core extensions have update sites ID = NULL when joining it with the update sites table for the extension itself or the package they belong to.
So you can say in your query you exclude those (type/element): 'file'/'joomla', 'package'/'pkg_en-GB', 'component'/'com_joomlaupdate'.
I will try to do that in my PR to your repo which I mentioned in my previous comment.
@pe7er Check PR pe7er#20 against your repo. It solves the problem mentioned by @AlexRed .
The other thing with core extensions check I will see what I can do.
@pe7er Regarding the core extensions I think it is really necessary to have my PR #16724 merged an then use it instead of checking for extention_id > 999. But for now I think it would be sufficient if you just change the 999 to 9999 so it fits to the auto increment of the extensions table (which starts at 10000 so everything below is a core extension for sure, and only core extensions which have been discovered by the discovery installer function will have larger IDs). I will add that as a commit to my previously mentioned PR to your repo.
@pe7er I added now a commit to my PR for your repository which replaces the unsafe check for extension_id > 999 by using the new JExtensionHelper class.
I haven't tested it yet here, but I will soon and let you know about the result. Of course if you test faster, you are welcome.
@pe7er Meanwhile I have tested my changes in my PR for your repo, and I corrected a silly mistake. Now it is ok. Core extensions are excluded now also if they have extension ID greater than 999 (or more), and for extensions which have a parent package which has an update site (e.g. German language pack), the warning is not shown.
So if you accept and merge my PR into your repo, I can test this one here with success.
But you might have to fix code style of my PR: The line with the condition in the view for check if the warning has to be displayed is a bit long after my changes, so you may have to do some line break.
@pe7er have you merged the changes from @richard67
@brianteeman I just merged @richard67 his code. Sorry for my delay!
I've just tested and noticed following:
If an update site is existing but disabled, it is not counted as missing, i.e. the badge is not shown in this case. It is only shown if the update site is really missing in database. @pe7er Is this by purpose? The testing instructions read as if it was by purpose. If so, then this observation is not an issue.
For components like e.g. OSMap which come with a component and a plugin and these do not belong to any parent package, this PR does not work. But I think that's by purpose, because I do not see any chance to find out in database that the OSMap content plugin is shipped with the OSMap component. The only chance would be to parse the component's manifest XML. Or do I miss something? If I don't miss something, then this 2nd observation is not an issue.
If both these observations turn out to be ok, i.e. not an issue with this PR, then I can mark my test result positive.
But because of the 2nd observation I think the badges intruduced with this PR could confuse the user, as it confused me at a first view.
@pe7er @brianteeman @Quy @AlexRed @AndyGaskell what do you think?
@richard67 Regarding your observation number 2. This can be determined by checking value of package_id
in #__extensions
table.
@wojsmol No, as I wrote, the OSMap stuff is organised differently, it does not use the package_id for its content plugin. What you write, checking the package_id column, is already implemented in this PR here (and I was the one who suggested that change, see history above).
I have tested this item
This PR works as described.
What it does not do is to show the badge if an update site is present in database but is disabled. It only shows the badge if the update site is missing in database. But I think this is as intended by this PR, so I do not count this as an error.
What this PR also does not do is not show the badge e.g. for plugins or modules which are installed with a component and not with a package, so the package_id column is not set in the extensions table in database, but the plugin or module is updated together with the component by the online update, like it is e.g. with the OSMap Component and its content plugin. But this is also not within the scope of this PR as I understand it, and so I don't count this as an error.
On the other hand, always showing the badge for those modules or plugins could confuse people and so maybe cause support requests in forums and issues in the tracker. The only chance to proper handle such stuff would be to parse the component's manifest XML, as far as I see. Or we can make component creators make their stuff be delivered in a package or set the package_id column at least.
Maybe the PLT or some regular maintainers should have a look on it and decide if that is ok or not.
Or we can make component creators make their stuff be delivered in a package or set the package_id column at least.
Extension developers can't be forced to repackage their extensions because of us. We can't account for every possible distribution model nor should we, so this scenario can be thrown into a list of known issues with a "won't fix" label.
Rethinking this it actually feels like it could be problematic or send mixed signals for extensions which are one off custom solutions for a site or client. For the joomla.org
backend we'd have several sites where this error badge would show up in error but it's not really a problem because these are our extensions and we aren't publishing update servers for them. There's a part of me that almost wants to suggest closing this without accepting it between that and the list of potential issues with how different extensions are packaged.
If there's an alternative that doesn't make it come across as an extension is broken or otherwise bad because it doesn't have an update site, that should be considered. But looking fresh on this I honestly don't feel like this is a good idea.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-09-25 13:46:16 |
Closed_By | ⇒ | pe7er |
Oops, I used tabs but my reinstalled IDE converted them to non visible spaces :-(
Thank you @Quy + @richard67 for your comments!
I've made all the changes that you've recommended.