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

[3.10] Correctly handle multiple critical extensions within an package that is joomla 4 compatible #34776

Merged
merged 10 commits into from
Jul 22, 2021

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Jul 13, 2021

Pull Request for Issue #34599 cc @nikosdion @brianteeman

Summary of Changes

Correctly handle two critical extensions within an package that is joomla 4 compatible

Testing Instructions

Install latest akeeba, enable the system plugin (backup on update) and the action log plugin.
point your update server to Joomla 4 (custom URL: https://update.joomla.org/core/test/310to4_list.xml)
apply this PR
clear the browser / JS cache.

Actual result BEFORE applying this Pull Request

The pre upgrade checker shows a message about potencial not compatible plugins within that package

Expected result AFTER applying this Pull Request

The pre update checker shows no message about that plugins cause they are compatible.

Documentation Changes Required

none

I have touched JS code here

I'm by far not an expert in JS so please suggest changes to my JS when i just did it wrong. Thanks!

@brianteeman
Copy link
Contributor

image

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 3026534


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

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

WTF. Just to be sure did you cleared cache cause i changed JS files?

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

image

Its working good here

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

With both "critical" plugins enabled
image

@wilsonge
Copy link
Contributor

Screenshot 2021-07-13 at 23 31 32

Don't have time to debug it tonight I'm afraid but I can reproduce brian's negative test

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

Don't have time to debug it tonight I'm afraid but I can reproduce brian's negative test

Thats a differen issue cause there it claims not to be compatible. Can you double check the update server you are pointing too?

@wilsonge
Copy link
Contributor

wilsonge commented Jul 13, 2021

Sorry my fault. I'd set it back to 3.10 as an update server rather than 4. For 4.0 as an update server indeed it indeed does work.

Screenshot 2021-07-13 at 23 36 09

We also need to fix the issue by the looks of it of the multiple plugins giving messages in the above screenshot where the server pointed to 3.10 (should only be one message for akeeba as a whole). but guess that can be a separate PR.

@wilsonge
Copy link
Contributor

I have tested this item ✅ successfully on 7b3f6af

Works for me


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

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

We also need to fix the issue by the looks of it of the multiple plugins giving messages in the above screenshot where the server pointed to 3.10 (should only be one message for akeeba as a whole). but guess that can be a separate PR.

This is the issue with the update server where it claims to be not compatible with 3.10 but ony 3.9 and 4.0 nothing I see that could be fixed on our side other than "guessing" whether an extension is compatible. But we also dont do that for updates so i dont think we should do it here and give even more confusion.

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

We also need to fix the issue by the looks of it of the multiple plugins giving messages in the above screenshot where the server pointed to 3.10 (should only be one message for akeeba as a whole). but guess that can be a separate PR.

Ah sorry got it wrong. I can not reproduce that problem, i have added "clear cache" to the test instructions does that work for you? Can you still reproduce the double messages?

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

Ok found another issue in that code.. Will try again to fix it with my limited JS skills.

@brianteeman
Copy link
Contributor

verified 1000% that the new js is being loaded

@brianteeman
Copy link
Contributor

I have been able to work out why our test results differ and maybe it help you solve the bug.

With just akeeba backup pro installed - no errors
with admintools pro also installed - errors

clearly a bug there

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

verified 1000% that the new js is being loaded

Hmm I have just pushed another hack in lack of better JS skills. I could not reproduce the issue with the double labels but maybe thats fixing it too?

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 13, 2021

With just akeeba backup pro installed - no errors
with admintools pro also installed - errors

I have no access to the pro versions to test with. Has the "free" version the same issue?

With the latest changes and "just" the free versions it looks like this:
image

@brianteeman
Copy link
Contributor

seems to work ok now

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 14, 2021

seems to work ok now

Thats great to hear. Thanks!

@zero-24 zero-24 changed the title [3.10] Correctly handle two critical extensions within an package that is joomla 4 compatible [3.10] Correctly handle multiblr critical extensions within an package that is joomla 4 compatible Jul 14, 2021
@zero-24 zero-24 changed the title [3.10] Correctly handle multiblr critical extensions within an package that is joomla 4 compatible [3.10] Correctly handle multible critical extensions within an package that is joomla 4 compatible Jul 14, 2021
@zero-24 zero-24 changed the title [3.10] Correctly handle multible critical extensions within an package that is joomla 4 compatible [3.10] Correctly handle multiple critical extensions within an package that is joomla 4 compatible Jul 14, 2021
@Fedik
Copy link
Member

Fedik commented Jul 14, 2021

a bit more readable will be:

var pluginInfo;
for (var i = PreUpdateChecker.nonCoreCriticalPlugins.length - 1; i >= 0; i--)
{
  pluginInfo = PreUpdateChecker.nonCoreCriticalPlugins[i];
  
  if (pluginInfo.package_id == extensionId || pluginInfo.extension_id == extensionId)
  {
    $('#plg_' + pluginInfo.extension_id).remove();
    PreUpdateChecker.nonCoreCriticalPlugins.splice(i, 1);
  }
}

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 14, 2021

Applied here thanks @Fedik 6b8a785

@nikosdion
Copy link
Contributor

Maybe a stupid question. Why don't you use forEach instead? Isn't this file going through Babel anyway? Also, why are we using jQuery when the promise is that Joomla core doesn't use it anymore?

I am confused by this JavaScript. It seems to be inconsistent with everything else in the core. I was shouted at for far lesser transgressions when I first tried to contribute my WebAuthn plugin and that was before Joomla made a public statement that it uses vanilla JavaScript in the core...

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 14, 2021

Maybe a stupid question. Why don't you use forEach instead? Isn't this file going through Babel anyway? Also, why are we using jQuery when the promise is that Joomla core doesn't use it anymore?

This is 3.10 where the migration to native JS has not been done yet. I'm not sure what you mean by "going througth babel anyway"? Just to be clear this patch here is for 3.10 not 4 where native JS will be used once merged up.

@nikosdion
Copy link
Contributor

Okay, so you are writing the same thing slightly differently twice, once for Joomla 3.10 and once for Joomla 4.0? Sorry, it just seemed a wasteful approach since you have to write and debug the same thing twice, written differently. If it floats your boat...

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 14, 2021

As suggested by @nikosdion this now also updated the "more information" css class from important to indo and looks like this than:
image

@brianteeman
Copy link
Contributor

Please check the js errors

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 14, 2021

Restarted drone.

@nikosdion
Copy link
Contributor

I have tested this item ✅ successfully on f059223

Tested by installing the upcoming versions of Akeeba Backup Core (8.0.7) and Admin Tools Core (6.1.0). I enabled the actionlog plugins on both extensions; Admin Tools also has a system plugin. I changed libraries/src/Version.php to pretend that Joomla is still 3.10.0-alpha8 to make it show me an update. I am also using private update XML files for my extensions which mark the upcoming versions as Joomla 3.10 compatible.

Before the patch: Joomla tells me the extensions are not compatible with Joomla 3.10 and More Info tells me to upgrade to... the installed version. That was under No Update Required. I also see the scary potential update problem message on both extensions, twice in Admin Tools which has two plugins.

After the patch: WOO-HOO! No more complains from Joomla. It tells me no update is required for these extensions.

I am happy with the way this works now.

Side note: I had a client last week who extracted the package ZIP file and installed each extension separately. Two weeks ago I had a client who was using Discover to install the plugins which appeared uninstalled after some failed database surgery. Right now, the Joomla Update component assumes that all extensions will either belong to a package OR have an update site. In both of these clients' cases — sadly, they are far from being the only ones doing things like that — this will fail. I can foresee a torrent of support requests. My current solution is to have my component check for a missing package extension and if so create it. I also check the extensions listed in a package's manifest and have it ‘adopt’ the ‘orphan’ extensions which appear in the package's manifest. The latter should really be part of Joomla itself, probably as a com_installer view in the same way we have the Database view (and its Fix button). Ideally, each extension should also declare if it is supposed to belong to a package and which package is that so that Joomla Update can provide better context to the user instead of misleading the user into believing the developer is doing something wrong and putting the onus on the developer to provide support for what is a self-inflicted issue as a result of Joomla allowing the user to do things it clearly DOES NOT want them to do. Should I open another issue about this or would you rather email me to request more info so you can discuss it with George?


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

@brianteeman
Copy link
Contributor

response to the side note. the same thing happens with languages that are discovered and not installed

@zero-24
Copy link
Contributor Author

zero-24 commented Jul 19, 2021

@nikosdion please open a new issue for that. So this can be tracked, when you have code feel free to propose a PR too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants