[#12184] - Tidy writeDynaList()
- Fixed in Code Base
- 20 Apr 2018
- Medium
- Build: staging
- # 12184
- Diff
- milux:staging
- Success hound No violations found. Woof! Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Failure continuous-integration/appveyor/pr AppVeyor build failed Details
- Success continuous-integration/drone/pr the build was successful Details
User tests: Successful: Unsuccessful:
Summary of Changes
Originally, I tried to get rid of writeDynaList() entirely (because of the awful document.writeln()) and do it all in PHP code. I was quite successful, but then I realized that the function was now used from JS code, so instead I cleaned up the code for the one and only place where this function is called now,
The first of the two commits includes excessive explanations how the function was simplified, second one is then just plain code and JSDoc.
Testing Instructions
none
Documentation Changes Required
none
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript |
| Labels |
Added:
?
|
||
Production version of core.js broke my site because of writedynalist() and it's use of writeln. Updating to the 4.0 version of core.js fixed this.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12184.
Well, most likely something else is broken now, but maybe you won't recognize it...
Are you using some aggressive resource optimizer? Like JCHOptimize with advanced settings?
@brianteeman I see the point in Joomla 3.x living for at least the next three years, issues for this version shouldn't be closed right away. Since this issue is "only improving" code (which definitely is important, too), an there is no testing instruction provided, this issue will most probably never get tested (as we can see on the last comment about one year ago.
@milux Can you provide a testing scenario, so that these changes can be tested?
| Status | Pending | ⇒ | Information Required |
Any use of this function should do the job.
In its latest form, this is nothing more than a small code cleanup.
It should be semantically equivalent. If you can't confirm semantic equivalence by simply comparing the code, I'm comfortable with this being abandoned.
@milux As far as I can see, your code seems to do the same thing, and it clearly reduced the mess in the code. However, only code looking like it does the same, it doesn't automatically mean it really does the same. Therefore, testing is needed.
Can you tell me where this code is used in Joomla, so that I can test that the changes do not affect the functionalities?
I have tested this item
Still works as expected.
@icampus
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12184.
I have tested this item
Works exactly like before
I went in the backend to "Extensions" -> "Modules" and there to one random entry. On the right side you find Ordering. Below that is a dropdown menu. This code runs that. An before and after the patch its still functioning.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12184.
| Status | Information Required | ⇒ | Ready to Commit |
RTC after two successful tests.
The compressed file needs updating too.
| Labels |
Added:
?
|
||
I updated the branch and added a compressed version of the most recent core-uncompressed.js
Is that what you asked for?
Why are the compressed versions not automatically generated from the uncompressed ones? That would be far less error-prone imho...
I missed the comment about it being updated and apparently scanned over it when looking over RTC lists. Don't let me forget about it after next week's release.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-04-20 20:34:29 |
| Closed_By | ⇒ | rdeutz |
@dgt41: More happy with the most recent commit?