Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Script options without inline JavaScript #11671

Closed
wants to merge 13 commits into from
Closed

[RFC] Script options without inline JavaScript #11671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
47f5c97
Joomla.getOptions and a script options without inline JavaScript
Fedik Aug 18, 2016
6f6c6f7
Options rendering
Fedik Aug 18, 2016
52df008
IE8 compatibility
Fedik Aug 19, 2016
14151a3
Correct place to call 'behavior.core'
Fedik Aug 19, 2016
fdcb48e
Use correct mime type
Fedik Aug 19, 2016
1fc35e5
add @since __DEPLOY_VERSION__
Fedik Aug 19, 2016
be7c703
Change position for call 'behavior.core'
Fedik Aug 19, 2016
f48a573
Change order for the scripts options rendering, to be able access the…
Fedik Aug 21, 2016
dae97f2
Respect to Boolean `false`
Fedik Aug 21, 2016
0b02d96
Testing for Joomla.getOptions
Fedik Aug 21, 2016
cfe2bed
Joomla.loadOptions allows to load a dynamically added options
Fedik Aug 21, 2016
b528028
Update testing for Joomla.getOptions
Fedik Aug 21, 2016
a97c69d
Fix IE8 compatibility
Fedik Aug 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 20 additions & 30 deletions libraries/joomla/document/renderer/html/head.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ public function fetchHead($document)
$document->_metaTags['name']['tags'] = implode(', ', $tagsHelper->getTagNames($document->_metaTags['name']['tags']));
}

if ($document->getScriptOptions())
{
JHtml::_('behavior.core');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is going to be considered this load statement should be before the compile event otherwise you've made it impossible for those who insist on unsetting core media to unset this call's dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will be laugh, but on start I made exactly as you just said,
but then thought, if ScriptOptions added while 'onBeforeCompileHead', but was empty before, then we got a problem 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just placing the if part between lines 57-58 should be enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already problems when people insist on unsetting the core loaded media (JCaption, MooTools, and core.js are the common ones I see already). We give people too powerful of a toolkit, not our fault when they abuse them. The only way to "fix" this is to make JDocument's assets APIs immutable (once it's in it's in unless you're getting your hands dirty with Reflection).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem if someone unset core.js, but ScriptOptions not empty, then the code Joomla.getOptions() will be broken.

Copy link
Member Author

@Fedik Fedik Aug 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is what is going on here 😉
behavior.core will be called only if $document->getScriptOptions() not empty

Copy link
Member Author

@Fedik Fedik Aug 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so for unset core.js you need to make empty $doc->scriptOptions
that will lead to broken site 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here isn't unique to the ScriptOptions. So I don't agree with having a media load statement at a point after we allow the user to manipulate what would be loaded because it's essentially giving preferential treatment to that specific file.

Either way this issue is bigger than this PR. For now I would say move it before the event and just like every other broken JavaScript statement that lives on sites because people are unloading dependencies, live with it or stop unloading crap.

Copy link
Contributor

@andrepereiradasilva andrepereiradasilva Aug 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with mbabker. if people unload core.js well ... it's their choice ... so they have to deal with the consequences.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, no problem, I will change it

}

// Trigger the onBeforeCompileHead event
$app = JFactory::getApplication();
$app->triggerEvent('onBeforeCompileHead');
Expand Down Expand Up @@ -184,6 +189,21 @@ public function fetchHead($document)
$buffer .= $tab . '</style>' . $lnEnd;
}

// Generate scripts options
$scriptOptions = $document->getScriptOptions();

if (!empty($scriptOptions))
{
$buffer .= $tab . '<script type="application/json" class="joomla-script-options new">';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

<script type="application/json" id="joomla-script-options" data-loaded="0">

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it require more action 😉
and HTML do not allow multiple elements with same id on the page, so I changed to class

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok them, didn't notice you needed multiple elements


$prettyPrint = (JDEBUG && defined('JSON_PRETTY_PRINT') ? JSON_PRETTY_PRINT : false);
$jsonOptions = json_encode($scriptOptions, $prettyPrint);
$jsonOptions = $jsonOptions ? $jsonOptions : '{}';

$buffer .= $jsonOptions;
$buffer .= '</script>' . $lnEnd;
}

$defaultJsMimes = array('text/javascript', 'application/javascript', 'text/x-javascript', 'application/x-javascript');

// Generate script file links
Expand Down Expand Up @@ -219,36 +239,6 @@ public function fetchHead($document)
$buffer .= '></script>' . $lnEnd;
}

// Generate scripts options
$scriptOptions = $document->getScriptOptions();

if (!empty($scriptOptions))
{
$buffer .= $tab . '<script type="text/javascript">' . $lnEnd;

// This is for full XHTML support.
if ($document->_mime != 'text/html')
{
$buffer .= $tab . $tab . '//<![CDATA[' . $lnEnd;
}

$pretyPrint = (JDEBUG && defined('JSON_PRETTY_PRINT') ? JSON_PRETTY_PRINT : false);
$jsonOptions = json_encode($scriptOptions, $pretyPrint);
$jsonOptions = $jsonOptions ? $jsonOptions : '{}';

// TODO: use .extend(Joomla.optionsStorage, options) when it will be safe
$buffer .= $tab . 'var Joomla = Joomla || {};' . $lnEnd;
$buffer .= $tab . 'Joomla.optionsStorage = ' . $jsonOptions . ';' . $lnEnd;

// See above note
if ($document->_mime != 'text/html')
{
$buffer .= $tab . $tab . '//]]>' . $lnEnd;
}

$buffer .= $tab . '</script>' . $lnEnd;
}

// Generate script declarations
foreach ($document->_script as $type => $content)
{
Expand Down
70 changes: 69 additions & 1 deletion media/system/js/core-uncompressed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,74 @@ Joomla.editors.instances = Joomla.editors.instances || {};
}
};

/**
* Joomla options storage
*
* @type {{}}
*
* @since __DEPLOY_VERSION__
*/
Joomla.optionsStorage = Joomla.optionsStorage || null;

/**
* Get script(s) options
*
* @param {String} key Name in Storage
* @param mixed def Default value if nothing found
*
* @return mixed
*
* @since __DEPLOY_VERSION__
*/
Joomla.getOptions = function( key, def ) {
// Load options if they not exists
if (!Joomla.optionsStorage) {
Joomla.loadOptions();
}

return Joomla.optionsStorage[key] !== undefined ? Joomla.optionsStorage[key] : def;
};

/**
* Load new options from given options object or from Element
*
* @param {Object|undefined} options The options object to load. Eg {"com_foobar" : {"option1": 1, "option2": 2}}
*
* @since __DEPLOY_VERSION__
*/
Joomla.loadOptions = function( options ) {
// Load form the script container
if (!options) {
var elements = document.querySelectorAll('.joomla-script-options.new'),
str, element, option;

for (var i = 0, l = elements.length; i < l; i++) {
element = elements[i];
str = element.text || element.textContent;
option = JSON.parse(str);

option ? Joomla.loadOptions(option) : null;

element.className = element.className.replace(' new', ' loaded');
}

return;
}

// Initial loading
if (!Joomla.optionsStorage) {
Joomla.optionsStorage = options;
}
// Merge with existing
else {
for (var p in options) {
if (options.hasOwnProperty(p)) {
Joomla.optionsStorage[p] = options[p];
}
}
}
};

/**
* Method to replace all request tokens on the page with a new one.
* Used in Joomla Installation
Expand Down Expand Up @@ -614,7 +682,7 @@ Joomla.editors.instances = Joomla.editors.instances || {};
parentElement.appendChild(loadingDiv);
}
// Show or hide the layer.
else
else
{
if (!document.getElementById('loading-logo'))
{
Expand Down
2 changes: 1 addition & 1 deletion media/system/js/core.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions tests/javascript/core/fixtures/fixture.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,11 @@
<form id="table-ordering-test-form">
</form>
</div>
<div id="get-options">
<script type="application/json" class="joomla-script-options new">{
"com_foobar": ["my options"],
"com_foobar2": "Alert message!",
"com_foobar3": false
}</script>
</div>
</div>
Loading