Ability to add other attributes to the script tag by andrepereiradasilva · Pull Request #8540 · joomla/joomla-cms

andrepereiradasilva · 2015-11-25T22:45:43Z

Description

This PR adds the ability to add more attributes to the script tag using JDocument addScript or addScriptVersion methods.

How to test

Add a external js script like, some examples:

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

JFactory::getDocument()->addScript('/path/to/external/jsfile.js', '', false, false, array('id' => 'scriptid', 'data-test' => 1));

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

JFactory::getDocument()->addScript('/path/to/external/jsfile.js', '', false, false, array('data-attrib' => 5));

JFactory::getDocument()->addScript('/path/to/external/jsfile.js', 'text/javascript', true, false, array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));

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')));

Load the page and check the html code. The script tag should be rendered with the attribute(s) added.

B/C

None that i know of.

dgrammatiko · 2015-11-26T12:48:08Z

👍
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?

wilsonge · 2015-11-26T13:57:35Z

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

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

andrepereiradasilva · 2015-11-26T14:19:30Z

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?

andrepereiradasilva · 2015-11-26T18:29:57Z

@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 · 2015-11-26T19:07:52Z

libraries/joomla/document/html/renderer/head.php

Couple notes:
First: this code will fail when $attrib_value is Object eg stdClass, so I would suggest test for is_scalar($attrib_value).
Second: need to be sure that both $attribute and $attribute_value is escaped.

Fedik · 2015-11-27T15:36:39Z

libraries/joomla/document/html/renderer/head.php

still have doubt here,
did you tried how it will work if $attrib_value contain '? 😉
more safe would be ="' . htmlspecialchars(json_encode($attrib_value)) . '"'

addslashes() will do that or am I missing something here?

@DGT41 htmlspecialchars more safe here 😉

andrepereiradasilva · 2015-11-27T16:45:50Z

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

Fedik · 2015-11-27T17:07:03Z

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

joomla-cms-bot · 2016-01-17T21:38:13Z

This PR has received new commits.

CC: @anibalsanchez, @DGT41

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8540.}

joomla-cms-bot · 2016-01-17T21:39:01Z

This PR has received new commits.

CC: @anibalsanchez, @DGT41

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8540.}

andrepereiradasilva · 2016-01-17T21:55:57Z

solved conflicts after JDocumentRenderer refactoring.

anibalsanchez · 2016-01-27T10:20:34Z

I have tested this item ✅ successfully on 4697be7

It works OK

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8540.}

dgrammatiko · 2016-01-27T19:36:52Z

@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 😄

mbabker · 2016-01-27T19:47:56Z

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.

andrepereiradasilva · 2016-01-28T00:06:53Z

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.

dgrammatiko · 2016-01-28T00:12:51Z

@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...

mbabker · 2016-01-28T00:15:17Z

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.

dgrammatiko · 2016-01-28T00:20:44Z

@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...

roland-d · 2016-06-24T21:12:18Z

@andrepereiradasilva Any update on this PR?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8540.}

andrepereiradasilva · 2016-07-03T11:44:00Z

@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

dgrammatiko · 2016-07-18T20:58:41Z

I have tested this item ✅ successfully on 4697be7

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8540.}

dgrammatiko · 2016-07-18T21:03:51Z

@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!

dgrammatiko · 2016-07-18T21:26:23Z

libraries/joomla/document/document.php

+		$this->_scripts[$url]['mime']    = $type;
+		$this->_scripts[$url]['defer']   = $defer;
+		$this->_scripts[$url]['async']   = $async;
+		$this->_scripts[$url]['attribs'] = $attribs;


@andrepereiradasilva how about:

if (!empty($attribs)) { if (!isset($this->_scripts[$url]) || (isset($this->_scripts[$url]) && empty($this->_scripts[$url]['attribs']))) { $this->_scripts[$url]['attribs'] = $attribs; } if (isset($this->_scripts[$url]) && !empty($this->_scripts[$url]['attribs'])) { $this->_scripts[$url]['attribs'] = array_merge($this->_scripts[$url]['attribs'], $attribs); } }

@mbabker since $attribs is newly introduced there is no B/C for what I'm proposing above, right?

actually that a good idea (i prefer with array_replace)

also i think i shoulnd' type hint this as an array.

i will try to check this better this weekend.

For the most flexibility attribs should be an array. That way it's easy to add (and remove since the whole thing's stored in a public var and we have the onBeforeCompileHead event) attributes. Whereas when it's a string like some places support its harder to deal with that stuff.

andrepereiradasilva · 2016-07-24T22:58:33Z

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.

andrepereiradasilva · 2016-07-28T14:40:41Z

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

brianteeman · 2016-07-28T14:42:00Z

Removed the milestone

andrepereirasilva added 2 commits November 23, 2015 21:52

Merge remote-tracking branch 'refs/remotes/joomla/staging' into staging

51113e7

Allows to add more attributes do script tag (e.g. data-*)

a751fa6

joomla-cms-bot added the PR-staging label Nov 25, 2015

cs - now they align

9e0e47b

andrepereiradasilva mentioned this pull request Nov 25, 2015

Look ma no addScriptDeclaration #8278

Merged

andrepereirasilva added 3 commits November 26, 2015 17:41

typeint array

878653a

Also support attribs in JHtml

634f673

forgotten typehint

2284f05

andrepereirasilva added 3 commits November 26, 2015 18:31

cs -more than 150 chars

70a7bbe

cs

930090b

cs

9b2665b

Fedik reviewed Nov 26, 2015
View reviewed changes

andrepereirasilva added 8 commits November 26, 2015 19:34

cs - this time?

b779832

cs - not yet ...

01b7c20

cs - and again

5730302

cs

00f0c47

cs ...

cba8221

escape vars

a38c457

thanks @Fedik

8681df2

cs

69e6489

andrepereiradasilva mentioned this pull request Nov 26, 2015

Move keepalive to external js #8545

Merged

Fedik reviewed Nov 27, 2015
View reviewed changes

andrepereirasilva added 2 commits November 27, 2015 16:34

escaping and checking also if is array

802b15c

ups, wrong version

804decf

no change in old head renderer

c81980d

cs space

4697be7

dgrammatiko mentioned this pull request Feb 18, 2016

No attrib for $doc->addScript() #9153

Closed

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 18, 2016

dgrammatiko reviewed Jul 18, 2016
View reviewed changes

andrepereiradasilva mentioned this pull request Jul 24, 2016

Add attribs to the script tag, add version to JHtml script methods and deprecate MD5SUM (alternative to 8540) #11289

Merged

roland-d added this to the Joomla 3.7.0 milestone Jul 26, 2016

andrepereiradasilva closed this Jul 28, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 28, 2016

andrepereiradasilva deleted the script-tag-attribs branch July 28, 2016 14:40

brianteeman removed this from the Joomla 3.7.0 milestone Jul 28, 2016

brianteeman added the RTC This Pull Request is Ready To Commit label Jul 28, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 28, 2016

Uh oh!

Conversation

andrepereiradasilva commented Nov 25, 2015 • edited by brianteeman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test

B/C

Uh oh!

dgrammatiko commented Nov 26, 2015

Uh oh!

wilsonge commented Nov 26, 2015

Uh oh!

andrepereiradasilva commented Nov 26, 2015

Additional info

Uh oh!

andrepereiradasilva commented Nov 26, 2015

Uh oh!

Fedik Nov 26, 2015

Choose a reason for hiding this comment

Uh oh!

Fedik Nov 27, 2015

Choose a reason for hiding this comment

Uh oh!

dgrammatiko Nov 27, 2015

Choose a reason for hiding this comment

Uh oh!

Fedik Nov 27, 2015

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Nov 27, 2015

Uh oh!

Fedik commented Nov 27, 2015

Uh oh!

joomla-cms-bot commented Jan 17, 2016

Uh oh!

joomla-cms-bot commented Jan 17, 2016

Uh oh!

andrepereiradasilva commented Jan 17, 2016

Uh oh!

anibalsanchez commented Jan 27, 2016

Uh oh!

dgrammatiko commented Jan 27, 2016

Uh oh!

mbabker commented Jan 27, 2016

Uh oh!

andrepereiradasilva commented Jan 28, 2016

Uh oh!

dgrammatiko commented Jan 28, 2016

Uh oh!

mbabker commented Jan 28, 2016

Uh oh!

dgrammatiko commented Jan 28, 2016

Uh oh!

roland-d commented Jun 24, 2016

Uh oh!

andrepereiradasilva commented Jul 3, 2016

Uh oh!

dgrammatiko commented Jul 18, 2016

Uh oh!

dgrammatiko commented Jul 18, 2016

Uh oh!

dgrammatiko Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgrammatiko Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Jul 19, 2016

Choose a reason for hiding this comment

Uh oh!

mbabker Jul 23, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva commented Jul 24, 2016

Uh oh!

andrepereiradasilva commented Jul 28, 2016

Uh oh!

brianteeman commented Jul 28, 2016

Uh oh!

andrepereiradasilva commented Nov 25, 2015 •

edited by brianteeman

Loading

dgrammatiko Jul 18, 2016 •

edited

Loading