? Pending

User tests: Successful: Unsuccessful:

avatar degobbis
degobbis
22 May 2021

Summary of Changes

Removed unneeded description from manifest_cache in json_encoded string as it breaks JS block in if unclean HTML code is set, preventing pre-update-check from running.

Testing Instructions

Install the T3-Framework from JoomlaArt (is a Template-Framework with BS3) and start the pre-update-check.

Actual result BEFORE applying this Pull Request

Browser Developer-Console tells JS errors and the pre-update-checker works not correctly.

Expected result AFTER applying this Pull Request

All works as expected.

avatar degobbis degobbis - open - 22 May 2021
avatar degobbis degobbis - change - 22 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2021
Category Administration com_joomlaupdate
avatar zero-24
zero-24 - comment - 24 May 2021

Isnt that an potenzial issue on other fields too? Would it make sense to clear all non relevant fields? Maybe @GeraintEdwards could take a look here too?

avatar degobbis degobbis - change - 25 May 2021
Labels Added: ?
avatar zero-24
zero-24 - comment - 28 May 2021

@degobbis please test: https://github.com/joomla/joomla-cms/compare/3.10-dev...zero-24:preupgrade_checker?expand=1 that should make sure we only us author and author URL that should also fix the issue right?

avatar degobbis
degobbis - comment - 28 May 2021

@zero-24 Take a look at my modifications, there are 2 places with the same approach.
No idea in which layout files this has to be adjusted.

But the problem is another, the information from the two methods are passed in some place as json in a JS variable. At this point an error occurs in the json if the description (as used for example in T3) is present. This results in the JS not being able to execute correctly. What information is still needed in the JS, I can not say, but the description is not used in any case. Therefore I decided to remove only this.

avatar GeraintEdwards
GeraintEdwards - comment - 28 May 2021

Apologies for the delay in responding here - we don't use the description field anywhere in the pre update checks so it makes sense to set the value to a blank.

It has the added benefit of saving a small amount of band width.

avatar zero-24
zero-24 - comment - 28 May 2021

All good but didnt we get the same issue when there are other fields like the name or licence with the same content? Thats the reason i started the other way arround and only loaded author and author url thst is used.

avatar degobbis
degobbis - comment - 29 May 2021

If I remember correctly, only in the description tag 'CDATA' can be used, right?
If so, that excludes the error in the other tags.

avatar Fedik
Fedik - comment - 29 May 2021

I think what @zero-24 suggest, is better, just to use what we need:

Thats the reason i started the other way arround and only loaded author and author url thst is used.

avatar degobbis
degobbis - comment - 29 May 2021

I basically agree, but this needs to be solved by someone who also has an overview of the entire functionality of the pre-update-checker.

My PR solves the described problem for now.

avatar degobbis
degobbis - comment - 29 May 2021

As I can see, we have the same issue in J4.
Is there a need to make a separate PR?

avatar zero-24
zero-24 - comment - 29 May 2021

As I can see, we have the same issue in J4.
Is there a need to make a separate PR?

No.

avatar zero-24
zero-24 - comment - 30 May 2021

Will take this PR here now as it is. More improvments can be made with later PRs. Thanks @degobbis

avatar zero-24 zero-24 - change - 30 May 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-30 13:35:01
Closed_By zero-24
avatar zero-24 zero-24 - close - 30 May 2021
avatar zero-24 zero-24 - merge - 30 May 2021

Add a Comment

Login with GitHub to post a comment