[4.0][WIP] More external scripts handled correctly #18912
Conversation
| defined('_JEXEC') or die; | ||
| ?> | ||
|
|
||
| use Joomla\CMS\HTML\HTMLHelper; |
There was a problem hiding this comment.
Are we using namespaced classes in layout views?
There was a problem hiding this comment.
Is there a reason not to?
There was a problem hiding this comment.
I'm sure it's fine, I just didn't think we were doing that and sticking with \JHtml, etc, in the layouts.
There was a problem hiding this comment.
yep, basically that part needs to go out of core, it' only used in couple places, not needed to be part of the core, core should be just the API
There was a problem hiding this comment.
Its fine to use the namespaced classes.
| @@ -0,0 +1,10 @@ | |||
| document.addEventListener('DOMContentLoaded', function(){ | |||
There was a problem hiding this comment.
Name of the file is incorrect admin-updaDe.js >> admin-update.js
| @@ -0,0 +1,10 @@ | |||
| document.addEventListener('DOMContentLoaded', function(){ | |||
There was a problem hiding this comment.
add standard comment block at top of the file
| el = document.getElementById("loading"), | ||
| position = outerDiv.getBoundingClientRect(); | ||
|
|
||
| el.style.top = position.top; |
There was a problem hiding this comment.
Keep as is for now, but I'll redo this so the styling is set via CSS rather than JS
There was a problem hiding this comment.
Was wondering: we do have a Joomla.loadingLayer method in core, why not using it here?
There was a problem hiding this comment.
Not sure, I'll do that in a separate PR cause even that function needs revisiting
| break; | ||
| default: | ||
| // An error occured -> show unknown error note | ||
| html = '<span class="label">' + Joomla.JText._('COM_JOOMLAUPDATE_VIEW_DEFAULT_EXTENSION_WARNING_UNKNOWN') + '</span>'; |
There was a problem hiding this comment.
class="label" >> class="badge badge-default". Or maybe throw an alert if you dont want to couple to a BS4 class
| } | ||
|
|
||
| if (uploadElement) { | ||
| uploadElement.addEventListener('change', function(e){ |
There was a problem hiding this comment.
Don't need to pass the event (e) as we're not using it
| var buttonElement= document.querySelector('button.submit'); | ||
|
|
||
| if (extractElement) { | ||
| extractElement.addEventListener('change', function(e){ |
There was a problem hiding this comment.
Don't need to pass the event (e) as we're not using it
| <?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_RECOMMENDED_SETTINGS_DESC'); ?> | ||
| </p> | ||
| <legend><?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_RECOMMENDED_SETTINGS'); ?></legend> | ||
| <p><?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_RECOMMENDED_SETTINGS_DESC'); ?></p> |
There was a problem hiding this comment.
use Text instead of JText
| </td> | ||
| <td><?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_DIRECTIVE'); ?></td> | ||
| <td><?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_RECOMMENDED'); ?></td> | ||
| <td><?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_ACTUAL'); ?></td> |
There was a problem hiding this comment.
use Text instead of JText
| <?php echo JText::_($setting->recommended ? 'JON' : 'JOFF'); ?> | ||
| </td> | ||
| <td><?php echo $setting->label; ?></td> | ||
| <td><?php echo JText::_($setting->recommended ? 'JON' : 'JOFF'); ?></td> |
There was a problem hiding this comment.
use Text instead of JText
| <td><?php echo JText::_($setting->recommended ? 'JON' : 'JOFF'); ?></td> | ||
| <td> | ||
| <span class="label label-<?php echo ($setting->state === $setting->recommended) ? 'success' : 'warning'; ?>"> | ||
| <?php echo JText::_($setting->state ? 'JON' : 'JOFF'); ?> |
There was a problem hiding this comment.
use Text instead of JText
| <legend> | ||
| <?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_EXTENSIONS'); ?> | ||
| </legend> | ||
| <legend><?php echo JText::_('COM_JOOMLAUPDATE_VIEW_DEFAULT_EXTENSIONS'); ?></legend> |
There was a problem hiding this comment.
use Text instead of JText
| <td> | ||
| <?php echo JText::_($extension->name); ?> | ||
| </td> | ||
| <td><?php echo JText::_($extension->name); ?></td> |
There was a problem hiding this comment.
use Text instead of JText
| var form = document.getElementById("uploadForm"); | ||
|
|
||
| // do field validation | ||
| if (form.install_package.value == "") { |
There was a problem hiding this comment.
CS: strict comparison, === over ==? Not sure about this in joomla context.
|
|
||
| // do field validation | ||
| if (form.install_package.value == "") { | ||
| alert(Joomla.JText._('COM_INSTALLER_MSG_INSTALL_PLEASE_SELECT_A_PACKAGE')); |
There was a problem hiding this comment.
we should rename Joomla.JText to Joomla.Text to align it with the backend.
There was a problem hiding this comment.
Don't forget to make a bc layer here, otherwise you will then break a lot of extensions.
There was a problem hiding this comment.
| * @param {function} callback | ||
| */ | ||
| PreUpdateChecker.checkCompatibility = function ($extension, callback) { | ||
| PreUpdateChecker.checkCompatibility = function (extension, callback) { |
There was a problem hiding this comment.
CS: shadowed variable extension.
this may lead to errors. (see below)
| $.getJSON(PreUpdateChecker.config.serverUrl, { | ||
| 'joomla-target-version': PreUpdateChecker.joomlaTargetVersion, | ||
| 'extension-id': $extension.data('extensionId') | ||
| 'extension-id': extension.data('extensionId') |
There was a problem hiding this comment.
here you are referencing the shadowed variable, but data is not a function.
|
Joomla cannot use npm for the aes script. Another solution needed here |
5 participants
Pull Request for Issue # .
Summary of Changes
For the Joomla update process there are 2 external libraries that are required, one for json parsing and another for encryption (AES).
For Json parsing we are switching from json2 to json3 (It is a drop-in replacement for JSON 2: https://bestiejs.github.io/json3/) because Douglas Crockford turned down our request for publishing json2 to NPM (douglascrockford/JSON-js#92)!
For AES we just bumped the version to the latest available.
And that is the part that takes care of the maintenance of the external scripts, but there is an ongoing work to drop jQuery from all the scripts. The later will be finished by the javascript team, but feel free to help with a PR.
Testing Instructions
Not ready for testing yet!
Expected result
Actual result
Documentation Changes Required
Nope. Housekeeping
Att @Fedik @C-Lodder @dneukirchen @yvesh