Some codestyle stuff already has been fixed with #33478 and #33479 . What remains is the strict comparisons in JS which @wilsonge mentioned and which need to be tested on PDO and MySQLi, see here: https://ci.joomla.org/joomla/joomla-cms/42900/1/21

Ah and yes, there is invalid html in the default_update.php, a closing td added in an else part for which before was no opening td. But this error seems to exist also in 3.10:

Update: Silly me. It's not an orphan closing tag, it's a single <td/>. So that's ok.

https://github.com/joomla/joomla-cms/blob/3.10-dev/administrator/components/com_joomlaupdate/views/default/tmpl/default_update.php#L185-L187

@wilsonge what is the type of extensionId ?

if (PreUpdateChecker.nonCoreCriticalPlugins[cpi].package_id == extensionId
            || PreUpdateChecker.nonCoreCriticalPlugins[cpi].extension_id == extensionId)

if it's a number you can rewrite this to something, like:
parseInt(extensionId, 10)

and make the comparison strict.

@dgrammatiko See #33487 . I can re-open if preferred by George.

@richard67 please reopen it, the solution looks solid

extensionId is a attribute value, so I think it is string. I think we can use

PreUpdateChecker.nonCoreCriticalPlugins[cpi].package_id.toString() === extensionId

Will that work?

Will that work?

It will work either way, just ensure that both sides are the same type ?

I think toString() is shorter (we do not need to cast both side to int. However, there are more problem than just that:

• I think foreach syntax is wrong https://github.com/joomla/joomla-cms/blob/4.0-dev/build/media_source/com_joomlaupdate/js/default.es6.js#L457
• There could be javascript error in case extension has special characters with this command https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_joomlaupdate/tmpl/joomlaupdate/default_preupdatecheck.php#L22
  ......

If we can be 100.00% sure that extensionData.element.getAttribute('data-extension-id') always results in a string, then I'd say we go the @joomdonation way.

This line

Factory::getDocument()->addScriptDeclaration("var nonCoreCriticalPlugins = '" . json_encode($this->nonCoreCriticalPlugins) . "';"); 

is totally unacceptable, Just use $doc->addScriptOptions() Grrrrr

@dgrammatiko Yes, that's right. We should use addScriptOptions() instead. The original code could cause eror in case plugins have special character as I said.

If we can be 100.00% sure that extensionData.element.getAttribute('data-extension-id') always results in a string

It always returns string, also the correct version of this line is: extensionData.element.dataset.extensionId Mind the capital i

I wanted to ask for performance comparison between getAttribute and dataset (just for education purpose :)).

I wanted to ask for performance comparison between getAttribute and dataset

For modern browsers, the performance should be the same (or in favour of dataset), but you can try it yourself:

https://www.measurethat.net/Benchmarks/Show/9389/0/modern-dataset-vs-old-getattribute-vs-jquery-data-vs-jq

One issue I am working on: The new database schema check added to the Pre-Update Check with PR #33080 shows false error because the new function has to be adapted to J4.

Update: George already preparing a fix.

See also #33497 . Sorry, not related.

I think this is good enough for now. We can create specific issues going forward