[#11289] - Add attribs to the script tag, add version to JHtml script methods and deprecate MD5SUM (alternative to 8540)
- Closed
- 9 Oct 2016
- Medium
- Build: 3.7.x
- # 11289
- Diff
- andrepereiradasilva:alternative-to-8540
- Success continuous-integration/drone the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Summary of Changes
This PR (alternative to #8540) adds the ability to add more attributes to the script tag using JDocument::addScript(), JDocument::addScriptVersion() and JHtml::script() methods. Also adds the ability to add IE conditional statements.
Also adds the ability to add the same attribs, script version and IE conditional statements when using JHtml::script() method.
Also deprecates MD5SUM and JDocument::addScriptVersion method.
How to test
Please test this in all possible scenarios.
Any problem will get scripts not loading.
- Use latest staging
- Apply patch
- Check backend and frontend are working fine
- Add external js scripts like to isis/protostar index.php, some examples:
// JDocument B/C tests
$this->addScript('/path/to/external/bcaddScript1.js');
$this->addScript('/path/to/external/bcaddScript2.js', '', true, false);
$this->addScript('/path/to/external/bcaddScript3.js', 'text/javascript', true, true);
$this->addScriptVersion('/path/to/external/bcaddScriptVersion1.js');
$this->addScriptVersion('/path/to/external/bcaddScriptVersion2.js', null, '', true, false);
$this->addScriptVersion('/path/to/external/bcaddScriptVersion3.js', 'version-string', 'text/javascript', true, true);
// JDocument new method tests
$this->addScript('/path/to/external/addScript0.js');
$this->addScript('/path/to/external/addScript1.js', array(), array('id' => 'scriptid'));
$this->addScript('/path/to/external/addScript2.js', array(), array('id' => 'scriptid', 'data-test' => 1));
$this->addScript('/path/to/external/addScript3.js', array(), array('data-attrib' => 'original-value'));
$this->addScript('/path/to/external/addScript4.js', array(), array('data-attrib' => 'final-value'));
$this->addScript('/path/to/external/addScript5.js', array(), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
$this->addScript('/path/to/external/addScript6.js', array('version' => 'auto'), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
$this->addScript('/path/to/external/addScript7.js', array('version' => 'version-string'), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
$this->addScript('/path/to/external/addScript8.js', array('version' => 'auto', 'conditional' => 'lf IE 9'), array('data-attrib' => array('item1' => 'value1', 'item2' => 'value2')));
// JHTML B/C Tests
JHtml::_('script', 'media/system/js/multiselect.js');
JHtml::_('script', 'system/repeatable.js', false, true, false, false, true);
// JHtml new method tests
JHtml::_('script', 'media/com_finder/js/indexer.js', array(), array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('script', 'system/progressbar.js', array('relative' => true), array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('script', 'system/permissions.js', array('relative' => true, 'version' => 'auto'), array('data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('script', 'system/sendtestmail.js', array('relative' => true, 'version' => 'version-string'), array('async' => 'async', 'defer' => 'defer', 'data-attrib' => array('item1' => 3, 'item2' => 'value2')));
JHtml::_('script', 'system/helpsite.js', array('relative' => true, 'version' => 'version-string', 'conditional' => 'lf IE 9'), array('async' => 'async', 'defer' => 'defer', 'data-attrib' => array('item1' => 3, 'item2' => 'value2')));- Check the HTML output and confirm all is fine
- do the same tests for anything else you can remember
- Do a code review.
Observations
From a B/C perspective i think there are no issues, but mantainers please check.
PRs depending on this one getting tested & merged
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
?
|
||
Can you add any attributes? For example I often use the integrity and
crossorigin tags
On 25 July 2016 at 10:01, Dimitri Grammatikogianni <notifications@github.com
wrote:
I am really sorry (because I like this approach) to say that this is not
b/c. The reason is that you change _scripts which is public:/** * Array of linked scripts * * @var array * @since 11.1 */ public $_scripts = array();—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11289 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8dCIkJ7cIlPBYO7agOsIMLKhlgW1ks5qZG12gaJpZM4JTsLM
.
Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
I am really sorry (because I like this approach) to say that this is not b/c. The reason is that you change _scripts which is public:
And why that is not B/C?
I used the public var, yes. But, notice, the var follows the same principles, so, i can be wrong, but how will this break B/C?
For more info, do a print_r($document->_scripts); in the head renderer before and after the patch.
Can you add any attributes? For example I often use the integrity and crossorigin tags
Yes, any html attribute to the script tag.
@andrepereiradasilva maybe it's the Monday morning effect. I'll have to do a real comparison of the vars, my imagination seems that fails me at the moment
ok. please check that and test when you have time. thanks.
At a glance seems fine. Should add JLog statements though.
ok thank you very much!
| Title |
|
||||||
| Title |
|
||||||
ok ready for tests
I have tested this item
works nicely
<script src="/media/system/js/progressbar.js" data-attrib="something nice"></script>
<script src="/media/system/js/permissions.js?5c98b192a5460c43157d762e1e88f0b7" data-attrib="{"item1":3,"item2":"value2"}"></script>
<script src="/media/system/js/sendtestmail.js?version-string" async="async" defer="defer" data-attrib="{"item1":3,"item2":"value2"}"></script>
```<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/11289">issues.joomla.org/joomla-cms/11289</a>.</sub>
@andrepereiradasilva Can you rebase this please?
ok, so for supporting also IE continional statements i did the following:
- Deprecated addScriptVersion
- Now JHtml::script(), JDocument::addScript have similiar method signatures.
- Moved the media version adding to JDocument Head Renderer
- And some other changes
Also all is now controlled by the options array parameter, where, depending on which method you used, you can set everything: conditional, version, relative paths, etc
So there is really no need to use addScript anymore, you can do everything with JHtml::script.
Please test and code review. Also check from a B/C perspective.
PR description is updated.
If this is accepted, similiar changes can be made for the css part.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11289.
Can someone test this one?
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11289.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11289.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-09-04 23:35:06 |
| Closed_By | ⇒ | wilsonge |
3.7 with the PR mentioned? Or have I misunderstood something?
you misunderstood, the other PR does not contain this PR, it depends on this PR to work.
So as it's is, what you merged in 3.7.x #11686 will not work.
without this PR, JHtml::script and JDocument::addScript does not support adding script with conditional statements (needed in #11686), among the other things in this PR description.
| Status | Closed | ⇒ | New |
| Closed_Date | 2016-09-04 23:35:03 | ⇒ | |
| Closed_By | wilsonge | ⇒ |
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
Thanks somehow missed this this morning
can anyone please test this?
thanks michael!
I have tested this item
Still good here
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11289.
| Labels |
Added:
?
|
||
Will test tomorrow!
The two PRs that are depending on this are merged so this here is missing in staging to work correct?
yes, it needs to be merged (after tests) in the 3.7.x branch .
it is 3.7 so it's needs to be re-based
| Labels |
Added:
?
Removed: ? |
||
@andrepereiradasilva i have changed the base can you fix the merge conflichts than?
I don't think there are any merge conflicts? And yes will try and find time tonight! Sorry really busy with work at the moment.
ok conflicts fixed, should be fine now. please test
Could we have one more successful test before this get's merged, please.
Looks good here but please confirm that:
data-attrib="{"item1":"value1","item2":"value2"}"></script>
Is correct, i mean the " 's ;)
Well, it has to be encoded, otherwise it would be data-attrib="{"item1":"value1","item2":"value2"}" and that would result in massively broken HTML.
I have tested this item
Thnaks I just want to be sure as i'm not a JS pro like you
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11289.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11289.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-10-09 07:40:50 |
| Closed_By | ⇒ | rdeutz |
| Labels |
Removed:
?
|
||
Removed RTC label
| Labels |
Removed:
?
|
||
I am really sorry (because I like this approach) to say that this is not b/c. The reason is that you change _scripts which is public: