Couple of things here:

• we need to make the relevant changes into JHtml::script();
• Unit testing is necessary for both functions
• since you already did this can you also make a check for a key-value 'toBottom' => true so we can introduce a way to append scripts to the end of the page?

on making the change to JHtml::script as well.

Also can we typehint the new parameter to ensure we have an array please?

Ok. I will check your comments when i have time.

Additional info

I'm also working in a more global solution to the scripts problems for the future. (check staging...andrepereiradasilva:scripts-dependencies).
Is a PoC and is not related to this PR, just to inform.

At least i have identified some things that the script manager needs:

• Ability to choose the js dependencies of a external/inline script and load all those depencies if not loaded yet
• Script should be executed in the order they are called (not external, then inline)
• One method to rule them all and add scripts and is a simple way
• Greater flexibility (add the attributes you want)
• Tottaly B/C (maintaing the current methods working)

@dgt41 what do you think?

@dgt41

we need to make the relevant changes into JHtml::script();

Done

Unit testing is necessary for both functions

And how we do this? Can you point to what needs to be changed/added?
Do you mean in this line?
https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/joomla/document/html/JDocumentHTMLTest.php#L50

since you already did this can you also make a check for a key-value 'toBottom' => true so we can introduce a way to append scripts to the end of the page?

Hum. This is outside se scope of this PR. See my previous comment.

@wilsonge

Also can we typehint the new parameter to ensure we have an array please?

Done

@Fedik, @dgt41 thanks for the feedback.
Can you check if everything is ok now?

looks fine now, from my point of view ... just need time to test

I have tested this item successfully on 804decf

Quotes works fine

Notice: Undefined index: attribs in /var/www/html/xtdirbs3/libraries/joomla/document/html/renderer/head.php on line 195

This PR has received new commits.

CC: @dgt41

Testing the detailed cases on PHP 5.6.14, most of them work Ok... but:

JFactory::getDocument()->addScriptVersion('/path/to/external/jsfile.js', 3, '', false, false, array('id' => 'scriptid'));

JHtml::_('script', 'system/jsfile.js', false, true, false, true, true, array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));

JHtml::_('script', 'system/jsfile.js', false, true, false, true, true, array('defer' => 'defer', 'data-attrib' => array('item1' => 3, 'item2' => 'value2')));

Catchable fatal error: Argument 5 passed to JDocument::addScript() must be of the type array, boolean given, called in /var/www/html/.../administrator/components/.... on line 3 and defined in /var/www/html/.../libraries/joomla/document/document.php on line 465

CORRECTION, only this case fails:

JFactory::getDocument()->addScriptVersion('/path/to/external/jsfile.js', 3, '', false, false, array('id' => 'scriptid'));<hr /><sub>This comment was created with the <a href="https://github.com/joomla/jissues">J!Tracker Application</a> at <a href="https://issues.joomla.org/tracker/joomla-cms/8540">issues.joomla.org/joomla-cms/8540</a>.</sub>

@anibalsanchez, there isn't a com_autotweet in Joomla
Check https://github.com/joomla/joomla-cms/tree/staging/administrator/components

It seems you have a component enabled in admin that has some kind of problem.

Can you check this file "/var/www/html/j35/administrator/components/com_autotweet/autotweet.php", on line 3

@andrepereiradasilva I have pasted the test in one of my testing sites. Sorry for the confussion. Next time, I will test on the core components. I have just removed the reference.

no problem

I have tested this item successfully on 95edef0

Tested in the latest staging, it works OK

This PR has received new commits.

CC: @anibalsanchez, @dgt41

This PR has received new commits.

CC: @anibalsanchez, @dgt41

This PR has received new commits.

CC: @anibalsanchez, @dgt41

solved conflicts after JDocumentRenderer refactoring.

I have tested this item successfully on 4697be7

It works OK

@andrepereiradasilva can't retest it right now, will do it in the week end. Just one thing that come across my mind:
If multiple times JHtml::_('script', 'some/script.js', .. ); is called then the data attributes should be merged. Is this the current functionality? If not then I guess we should make this change.
If this is already implemented, just discard this comment ????

That's something JDocument should be handling (and IIRC isn't). Some of the JHtml helpers have checks to ensure they're only loaded once, but that's the extent of that type of handling.

IIRC, and looking at the code it adds the same attribs to all the scripts.
But a test is needed.

Update: i misunderstood what you were saying.
I don't know if you call several times what happens. Will check when i have time.

@andrepereiradasilva let me explain what I am proposing here:
you have template that registers bootstrap.js and appends some data for tooltip
then a module is also trying to set some other attribs for tooltips
The last call should not overwrite the existing data attributes but rather merge it to the existing.
Ok the example might be stupid, but I hope it gives you some insights...

That's a totally separate patch (and borderline B/C concern) unrelated to this. Right now JDocument's add methods for script/stylesheet will overwrite in full an item when you add something with the same URL. This patch will only expose that bug further.

@mbabker if that's the case this functionality is too restricted and in some extend inefficient. I can imagine attaching different data-attr to bootstrap.js from different views (all of them rendered the same page). There must be a way around this...

@andrepereiradasilva Any update on this PR?

@roland-d the problem @dgt41 is talking about is because of the not so good JDocument storing and rendering the scripts.
This problem as already been discussed several times.

See, for instance, my comment in #10223 (comment)

This needs a full rework IMO and there are several PR proposing most of this changes needed.
But i guess it will be very difficult before 4.0 because of B/C

I have tested this item ✅ successfully on 4697be7

@roland-d my comment is not directly related to this PR and the solution should be done in another PR. I suggest to merge this as it will help eliminate a lot of the inline scripts, it's a huge improvement!

i think i have a better alternative for this that i think it's more flexible.
Made a new PR for this alternative method.
Please check #11289 . If ok will close this one.

please remove the RTC and label from this.
i think #11289 is a better way to do this so i'm going to close it

Removed the milestone