? ? Failure

User tests: Successful: Unsuccessful:

avatar andrepereiradasilva
andrepereiradasilva
28 Nov 2016

Summary of Changes

This makes com_installer update sites respect the protected status of the extensions, ie, when the extension is protected it will not be able to be enable/disable/delete their update sites.

TODO:

  • update sql to enable all protected extension update sites to have them etternaly disabled
  • figure out out to solve the core disabling update sites when not reachable

Testing Instructions

  1. Code review
  2. Apply patch
  3. Check in extensions manage which extensions are protected.
  4. Go to extensions -> manage -> update sites and check the update sites of those extensions are now protected (cannot be enabled or disabled nor deleted).
  5. Try to disable/enable/delete update sites of protected extensions. You should NOT be able to do it.
  6. Try to disable/enable/delete update sites of unprotected extensions. You should be able to do it.

Documentation Changes Required

None.

avatar andrepereiradasilva andrepereiradasilva - open - 28 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 28 Nov 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Nov 2016
Category Administration com_installer Language & Strings
avatar andrepereiradasilva andrepereiradasilva - change - 28 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 28 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 28 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 28 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 28 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 28 Nov 2016
avatar brianteeman
brianteeman - comment - 28 Nov 2016

Doesnt this mean that a "protected" extension can never be updated? This seems wrong to me. There are a lot of extensions that install as protected (maybe they shouldnt but they do)

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

? yes, of course it can be updated, you just can't disable/delete it's update site in the updates sites view.

use case: core joomla update site ... should never be allowed to be disabled/delete. Currently we have some hardcode stuff to not allow to delete some core extensions update sites.

This would extend that behaviour to any extensions declared as protected without the hardcoded core stuff.

Also visually this now should the lock icon as in extensions manage view.
image

avatar brianteeman
brianteeman - comment - 28 Nov 2016

Try to disable or change the core update channel and see what happens -
nothing :)

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

so it was a bad example because it seems that joomla update component overrides update sites behaviour and enables the joomla core update site if it's disabled. The same happens to Accredited joomla Translations.
Maybe this fallbacks was made because of this issue (update sites being disabled in the update sites view) or even because of the updates sites view didn't existed before.

Anyway,, the same applies to other protected extensions that don't have an hardcoded fallback.
For instance "Joomla! Update Component Update Site" or "Joomla! Extension Directory" or any other 3RD party protected extension.

The thing is, IMHO, we should not be using hardcoded fallbacks when there is a central place to manage the update sites. If they are not allowed to be disabled, the user should not be allowed to disable them.

avatar zero-24
zero-24 - comment - 28 Nov 2016

Btw. Some times it happens that the core disable the update server. (if it cant reach it for any reasons) so you cant re enable it than?

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

Btw. Some times it happens that the core disable the update server. (if it cant reach it for any reasons) so you cant re enable it than?

The core disables update servers from protected extensions? Why? Is there a reason for that?
For instance, will the core disable com_joomlaupdate update server if not reachable? And only be active if the user manually activate it again? This seems strange to me.

Anyway, without knowing the reasons behind that decision i really can't say much.

avatar mbabker
mbabker - comment - 28 Nov 2016

The core disables update servers from protected extensions? Why? Is there a reason for that?

It doesn't check an extension's status.

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

The question i have is should the update site be disabled when not reachable or, for instance, have a "ban" period of x minutes?

as you know, disabling an update site means the extension will not be searchable for updates, which in some cases can lead to security issues.
asking the user to manually go and enable the update site the core as disabled is the best solution for this?

Wouldn't it be better to have a "ban" period for that update site?

avatar mbabker
mbabker - comment - 28 Nov 2016

I have no insight why it's the way it is. All I know is if the update service can't reach a particular update site it disables it and that's the existing behavior you're fighting against. If you want to change it, have fun; I try to avoid touching the update libraries these days because right now all I want to do is rip them out and re-engineer them ground up.

avatar zero-24
zero-24 - comment - 28 Nov 2016

It get disabled if it got not reached to not slow down the update checking. There is no check about protected or not there. This was the reason that the updatesites view was added. So you can manually enable the sites.

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

It get disabled if it got not reached to not slow down the update checking. There is no check about protected or not there. This was the reason that the updatesites view was added. So you can manually enable the sites.

Ok. I'm understanding why this was done. But could that be solved without a permanent disable, ie, like i said in earlier posts an incremental "ban" (or inactive) period for the update site in question.
This because the lack of connection to the update site can be temporary and disabling an update site because of this temporary connection issue could not be best awnser for this.

Anyway since this happens also to protected extensions this invalidates this PR until that is figured out...

avatar andrepereiradasilva andrepereiradasilva - change - 28 Nov 2016
The description was changed
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 28 Nov 2016
avatar sovainfo
sovainfo - comment - 28 Nov 2016

@andrepereiradasilva Suggest you have a look at the PR that introduced the Update Sites: #3775

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

thanks! will have a look.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

ok, so after reading that i'm working on a solution without disabling updates sites on connection issues.

Basicly, instead of disabling the update site when there is a connection issue, added a new field in the update_sites database table called failed_attempts that count the number of consecutive failed attempts.

This will add an 1 day delay for each failed attempt in that update site.

When clearing update cache, it tries again, but does not reset the counter of failed attempts. that counter is only reseted when a success attempt is done.

This way the update sites are never disabled by core automatic process, is always a user choice to disable them. But, at the same time, problematic update sites are not always searched. Also a temporary connection issue does not make the update site being disabled.

See https://github.com/andrepereiradasilva/joomla-cms/pull/108/files

If this is ok i will make a PR exclusive for this and them there is no issue in this PR.

avatar andrepereiradasilva andrepereiradasilva - change - 29 Nov 2016
The description was changed
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 29 Nov 2016
avatar brianteeman
brianteeman - comment - 29 Nov 2016

So if I read this correctly this completely removes the automatic disabling
of a failed update server. If so then this is a problem. IIRC if an ext dev
changes their update server there is no way to update it on the site they
can only add a new update server. Which mens they are relying on the old
server being automatically disabled on the old and now failing server

On 29 November 2016 at 00:38, andrepereiradasilva notifications@github.com
wrote:

ok, so after reading that i'm working on a solution without disabling
updates sites on connection issues.

Basicly, instead of disabling the update site when there is a connection
issue, added a new field in the update_sites database table called
failed_attempts that count the number of consecutive failed attempts.

This will add an 1 day delay for each failed attempt in that update site.

When clearing update cache, it tries again, but does not reset the counter
of failed attempts. that counter is only reseted when a success attempt is
done.

This way the update sites are never disabled by core automatic process, is
always a user choice to disable them.

See https://github.com/andrepereiradasilva/joomla-cms/pull/108/files


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#13041 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8ehLtfEbe0XvCLEX_Yzd4K69PUdYks5rC3P-gaJpZM4K9Tia
.

--
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar mbabker
mbabker - comment - 29 Nov 2016

It has pros and cons. The behavior right now is it fails once and it just turns itself off until someone re-enables it. That's a pretty bad failover, especially if that one time is because the user's site is checking for updates at a time where the developer's site might be down for maintenance or a server related issue.

There should probably still be a disable completely behavior, but most definitely not after one attempt.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

There should probably still be a disable completely behavior, but most definitely not after one attempt.

ok, i see, i can change my proposal to: if not protected extension on 3 (or 5, or 10) failed attempted it disables the extension update site.

This way i think we can have "the best of both worlds":

  • protected extensions update sites would never be disabled but would have inactive periods - using as base for the inactive period the number of failed attempts.
  • non protected extensions update sites would also have inactive periods - using as base for the inactive period the number of failed attempts, but, on 3 (or 5, or 10) attempts the update site would be automatticly disabled.
avatar mbabker
mbabker - comment - 29 Nov 2016

if not protected extension

Well, I'm gonna ask the hard question now. What makes a protected extension so special that its update site shouldn't be disabled? The protected status as it is now (and the locked status if merged) only have to do with enable/disable actions and uninstalling the extension, nothing to do with an extension's updateability.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

Well, I'm gonna ask the hard question now.

Right, understand the doubt. 😉 i also have it.

The concept that i'm proposing here is to protected also the update site of an extension if the extension is protected.
In other words, i'm seing the extension as a whole, being the update site part of the extension, so in that concept it would/should be protected too if the extension is protected.
I see the core protected extensions as an example for this concept, but also 3RD party frameworks.

Anyway this PR is really a new concept and maybe should be titleded as RFC. (i changed now to RFC)
Of course, if people don't agree with this PR logic i can always drop it. no warm done.

Also right now he make a series of hardcoded stuff to prevent core update sites being deleted, the goal is to remove this hardcoded behaviour too. As discussed in the other PR the logic that applies to core extensions, could/should also be available for 3RD party.

avatar andrepereiradasilva andrepereiradasilva - change - 29 Nov 2016
Title
[com_installer updatesites] Do not allow to enable/disable/delete a protected extension update site
[RFC] [com_installer updatesites] Do not allow to enable/disable/delete a protected extension update site
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 29 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 29 Nov 2016
Title
[com_installer updatesites] Do not allow to enable/disable/delete a protected extension update site
[RFC] [com_installer updatesites] Do not allow to enable/disable/delete a protected extension update site
avatar mbabker
mbabker - comment - 29 Nov 2016

We don't even treat update sites as a native part of anything. Seriously. That "Extension - Joomla" plugin is what decides whether an update site should be added or removed (and very poorly I might add). The UI to "manage" those was a major afterthought introduced far too late (much needed, but still shows how this critical aspect of Joomla isn't fully completed).

Personally, I don't see update sites as part of an extension. They are an integration point into another part of the overall Joomla application. An extension is still fully functional with or without an update site defined or enabled. So to me it feels a bit awkward to extend the definition of a protected extension to include something that isn't really part of the extension or its native functionality.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

Seriously. That "Extension - Joomla" plugin is what decides whether an update site should be added or removed (and very poorly I might add).

Actually my 2 other PR (#13037 and #13040) protects that plugin so no one can disable it - currently this it's a big flaw in the system IMHO.

@mbabker ok, thanks, as always, for your most interresting comments. i have some doubts reggarding them:

  • you don't agree that updates sites can have the need to be protected? (i see that need at least for the core extensions). Or you agree they should be able to be protected, but not linked to extension protected status?
  • you agree the current situation of disabling the update sites on first failed connection is not the best solution, and maybe an inactive period with failed attempts count is probably a best solution for this?
avatar mbabker
mbabker - comment - 29 Nov 2016

you don't agree that updates sites can have the need to be protected? (i see that need at least for the core extensions). Or you agree they should be able to be protected, but not linked to extension protected status?

The "core" update channels (com_joomlaupdate, the CMS itself, and the language packages; we should just delete that never used JED channel already) probably should be able to be protected, but I'd consider putting those in a protected state as a separate action from putting their associated extensions in a protected state. Especially as the language list tries to offer all the languages based on one of the English language extensions' ID and the CMS' is reliant on another extension which should be removed (Joomla isn't an extension of Joomla).

you agree the current situation of disabling the update sites on first failed connection is not the best solution, and maybe an inactive period with failed attempts count is probably a best solution for this?

Yes.

avatar sovainfo
sovainfo - comment - 29 Nov 2016

Consider the whole idea of protecting pretty stupid! Not something you would expect from open source software!

Obviously, the update server shouldn't be disabled when unreachable. Just because it failed once, that doesn't even warrant it to skip checking the next time. Let alone disabling it. Would be nice to communicate how many times it failed and how long it wasn't able to check for updates. The super user would use that information to decide what to do, which is his job! Not the job of the software!!!!

Once that bug is fixed, the bug taking advantage of it needs fixing as well. Meaning that the change of an update server shouldn't result in two update servers. That is a bug that needs solving as well. Loosing the advantage of disabling the update server in that case is irrelevant, there shouldn't be two servers! Taking advantage of a bug should not stop fixing it. That is considered a bug that needs fixing as is the case here! Obviously, fixing update of update servers stops taking advantage of the first bug.

Rather than preventing super loosers to do the wrong things, the backend should allow the super users to do what they need to do. The administrator app allows to administer the site, which means the intelligence is with the user of it. Don't try to change it in a manager app, it is missing far too much intelligence. It doesn't meet the qualification of an expert system, not by a long shot!
Remember there is no cure for stupid!

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

Consider the whole idea of protecting pretty stupid! Not something you would expect from open source software!

First, i ask you to show more respect to other people ideas, either you agree with them or not, it's ok by me. As you for sure know, an opinion can be expressed without insulting other peoples ideas.

Anyway, back to the point, we have to agree in disagree on this.

Joomla IMHO shouldn't allow the user to disable the joomla core update server as, for instance, it does not allows him to disable com_content or com_menus components.

Obviously, the update server shouldn't be disabled when unreachable.

Seems obiuous after this discussion to me too, but the reality is like that for years and no one as submitted code to "fix" that.

avatar brianteeman
brianteeman - comment - 29 Nov 2016

I disagree. It was explained very well in the original PR why the update
server is disabled if it cannot be reached in the original PR
In the Extensions/update screen it will tell you if you have any disabled
update servers. Finally as stated before - many extensions will have
generated multiple update servers over time. Joomla provides no way for the
3pd to edit an update server only to add a new one. So of course I want
those to be disabled. Think about it. Think about the performance of
testing 20 failed update servers every single time

On 29 November 2016 at 20:44, andrepereiradasilva notifications@github.com
wrote:

Consider the whole idea of protecting pretty stupid! Not something you
would expect from open source software!

First, i ask you to show more respect to other people ideas, either you
agree with them or not, it's ok by me. As you for sure know, an opinion can
be expressed without insulting other peoples ideas.

Anyway, back to the point, we have to agree in disagree on this.

Joomla IMHO shouldn't allow the user to disable the joomla core update
server as, for instance, it does not allows him to disable com_content or
com_menus components.

Obviously, the update server shouldn't be disabled when unreachable.

Seems obiuous after this discussion to me too, but the reality is like
that for years and no one as sumitted code to "fix" that.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#13041 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VOKbeOXWksJ128pBfpZqFRPJu8_ks5rDI7LgaJpZM4K9Tia
.

--
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar sovainfo
sovainfo - comment - 29 Nov 2016

@andrepereiradasilva Don't know why you consider expressing my opinion as not being respectful, so may I return the favor and ask you to stick to the subject! If you do, I won't comment any further on your disrespectfull comment!

avatar brianteeman
brianteeman - comment - 29 Nov 2016

Gentleman - this may have to be locked

On 29 November 2016 at 20:53, sovainfo notifications@github.com wrote:

@andrepereiradasilva https://github.com/andrepereiradasilva Don't know
why you consider expressing my opinion as not being respectful, so may I
return the favor and ask you to stick to the subject! If you do, I won't
comment any further on your disrespectfull comment!


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#13041 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8WGTOBGISghm8bXUOSbpePN0_zs2ks5rDJDRgaJpZM4K9Tia
.

--
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

there is no need brian, let's just stick to the point here.

@brianteeman i understand the issue you are saying and that also needs to be solved.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

@sovainfo yes, it would be good to have the failed attempts count in the list view.
in your opinion, what do you think it's better:

  1. log failed attempts in a counter and reset counter when a success attempt
  2. log failed attempts in a counter and not reset count when a success attempt, and have another counter for the total of failed attempts for that update server

my opinion is that 1 is enough to have a perspective. but open to suggestions

avatar sovainfo
sovainfo - comment - 29 Nov 2016

Untill the bug in Joomla is fixed, the extension should make sure it doesn't leave an old invalid update servers.
Fortunately, the Extensions>Manage>Update servers allows you to remove them. Again, just a webmaster doing his job! Beats removing them manually from the database!

Too bad that the implemtation is this poor. You might as well remove them all and do everything the manual way, as you need to do for those not supporting the update server.

Assuming you are familiair with the trick to find out non-core extensions (order the extensions by id descending and take the ones above 10000. Note that there can be false positives) You need to keep those current!
This and the code in script.php to update the manifests are proof of the need for an indication for the type of extensions. Two situations that justify an attribute in #__extensions to mark the origin of it.
You might ask a professional data modeller that understands attribute normalization. Is it not time to stop abusing the relational database as ISAM files.

avatar sovainfo
sovainfo - comment - 29 Nov 2016

@andrepereiradasilva
Like the idea of communication failure, definitely beats silently disabling. Think that people would appreciate to know how many times it failed and how long ago the last check was. Would probably disable it based on that to further investigate and finally remove it if it turns out to be an outdated server.

Probably remove it also when the extension keeps causing multiple unresponsive update servers, might even reconsider continued use of that extension!

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

let me try to think for a while and try to get a solution.

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

@brianteeman reggarding the update sites change process this seems to happen here https://github.com/joomla/joomla-cms/blob/staging/plugins/extension/joomla/joomla.php#L231

One thing i think could be improved is: if the extension new update site name is the same as the old one but the update site location is different (ex: a change in update site url), joomla could update the update site record, instead of creating a new one.

Still if the extension have a different update site name, a new update site record will be created for that extension.

avatar sovainfo
sovainfo - comment - 30 Nov 2016

@andrepereiradasilva

Indeed that is the bad code causing multiple entries when the url changes. It completely ignores current servers and just adds one when the location (url) doesn't exist.
Obviously, it should remove the current servers for this extension not mentioned in the manifest.
My understanding is that the manifest declares the desired update servers. That means the update needs to detect those to be removed, those to be added, those to be left alone. Pretty standard algorithm when communicating changes this way!

avatar andrepereiradasilva
andrepereiradasilva - comment - 30 Nov 2016

@sovainfo conceptually i agree with you.
Now how to do that is a different question, but i'm investigating, as soon as i have something i will make a specific PR for that issue.

avatar sovainfo
sovainfo - comment - 30 Nov 2016

Not that difficult:

  • Determine current servers for this extension
  • Determine desired servers for this extension
  • ToBeRemove = array_diff ( current, desired)
  • ToBeAdded = array_diff ( desired, current)
  • Remove ToBeRemoved
  • Add ToBeAdded
  • Remove unused servers
avatar mbabker
mbabker - comment - 30 Nov 2016

Sadly when the plugin was written years ago adding update site support (I still don't get why this is in a plugin and not JInstaller itself; someone took the logic of decoupling core APIs a bit too far here) it wasn't written that smart and here we are today living with those decisions.

avatar mbabker
mbabker - comment - 27 May 2017

If this is something that's going to get seen through then it needs a new owner.

avatar franz-wohlkoenig franz-wohlkoenig - change - 27 May 2017
Status Pending Information Required
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jun 2017

If this PR get no Response, it will be closed at 23th July 2017.

avatar franz-wohlkoenig franz-wohlkoenig - change - 23 Jul 2017
Status Information Required Closed
Closed_Date 0000-00-00 00:00:00 2017-07-23 08:32:45
Closed_By franz-wohlkoenig
avatar joomla-cms-bot
joomla-cms-bot - comment - 23 Jul 2017
avatar joomla-cms-bot joomla-cms-bot - close - 23 Jul 2017
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jul 2017

closed as stated above.


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

Add a Comment

Login with GitHub to post a comment