[J3.7.x] Changes to addStyleSheetVersion, addStyleSheet, addScriptVersion, addScript, existing J3.6.x code is partly broken

[ggppdk]

Steps to reproduce the issue

Try to use:

JFactory::getDocument->addStyleSheetVersion('my.css', $version='3.1.1');
JFactory::getDocument->addStyleSheetVersion('my.js', $version='3.1.1');

Expected result

CSS and JS files will be added to the page, together their version string

Actual result

J3.6.x: It works
Staging: Partly broken

• assets are added but without the version string
• you get PHP warning

Warning: Illegal string offset 'version' in deve/joomla-cms/libraries/joomla/document/document.php on line 550
Warning: Illegal string offset 'version' in devel/joomla-cms/libraries/joomla/document/document.php on line 741

System information (as much as possible)

J3.6.x signatures

public function addStyleSheetVersion($url, $version = null, $type = "text/css", $media = null, $attribs = array())
public function addScriptVersion($url, $version = null, $type = "text/javascript", $defer = false, $async = false)

J3.7.x signatures

public function addStyleSheetVersion($url, $options = array(), $attribs = array())
public function addScriptVersion($url, $options = array(), $attribs = array())

At minimum the existing code of calling the method with the old way needs to work,
and there should be no PHP warnings

It should not be too difficult to detect the old way of calling the methods, and make them work

Additional comments

Also, in the rare case that someone both extends the JDocument, and also overrides the 2 methods, there will be some PHP warnings

[ggppdk]

I looked at the code in the

• i see that the code already tries to detect old way of calling

@andrepereiradasilva

if (!is_array($options) && !is_array($attribs))

should probably be: (in all 4 methods)

if (!is_array($options) || !is_array($attribs))

[ggppdk]

Also for the case of B/C of someone extending JDocument and redeclaring the methods:
addStyleSheet, addStyleSheetVersion
addScriptVersion, addScriptVersionVersion

• i think the method parameters, need to go back to the old default values so that they are B/C with anyone extending the classes and overriding the methods

Notes:

• we can keep the new parameter names, as these are local to the method and make no B/C break
• also no B/C break should be the removal of the extra parameters, (as long as we have code to handle them inside the method, and i see that this code is there), thus any extending class override these method can still declare the old method parameters without problems

To summarize, $options and $attribs need their old defaults to stay compatible
addStyleSheet($url, $options = array(), $attribs = array())
addStyleSheetVersion($url, $options = array(), $attribs = array())
addScriptVersion($url, $options = array(), $attribs = array())
addScriptVersionVersion($url, $options = array(), $attribs = array())

[EDIT]
The "compatiblity" way of using the methods, should be documented in the code (and in official documentation), now the code handles old way of calling, but does not document the previous signature

[andrepereiradasilva]

@ggppdk i can try and solve the warning issues - it's obvious a bug.

Reggarding potential B/C issues with the change with method signatures in all 6 methods (they were changed in JHtml too), i asked to check B/C policy in the first PR that proposed this change in method signatures (#11289).

If that are B/C problems in those PR they need all to be reverted.

For info, the sequence of PR that made this changes are:

• For JS: Add attribs to the script tag, add version to JHtml script methods and deprecate MD5SUM (alternative to 8540) #11289, Add script version to all js files and replace deprecated script signatures usage #12373, [3.7.x] addScript corrections after 11289 merge #12429, Add attribs to the script tag, add version to JHtml script methods and deprecate MD5SUM (alternative to 8540) #11289 and Add script version to all js files and replace deprecated script signatures usage #12373.
• For CSS: Add version and IE conditional statements to JHtml::stylesheet/JDocument::addStyleSheet methods and deprecate JDocument::addSyleSheetVersion() #12375, Add stylesheet version to all css files and replace deprecated stylesheet signatures usage #12720, Don't add media version if media version is empty (now for css) #12781, Add stylesheet version to all css files and replace deprecated stylesheet signatures usage #12720 and Add missing mediaversion to polyfill js files #12714.

Also all the merged PR that use this new PR method signatures would need to be corrected..
All all the js polyfill changes and all the PR that do inline js changes to use this new ie conditional option would needs to be changed/reverted.

(and probably more PR that i can't remember now)

So we need to be sure this is a B/C issue. PLT please advice.

[ggppdk]

Besides the small bug (wrong IF check, that is easy to fix)

the only B/C issue that might be a problem (or it may not be, are the default values) of the methods

• but now that i rethink of it , default values are not a B/C break at all,

i was confused with declaration of type for method parameters, which is not our case
i will test now

[ggppdk]

@andrepereiradasilva

I am sorry i was confused with declaration for variable types for method parameters,
which is not our case

I tested, no B/C break

Only the bug with PHP warning,
caused by the wrong IF check, needs to be fixed

if (!is_array($options) && !is_array($attribs))

Something like this should be enough:

if (!is_array($options))

[andrepereiradasilva]

np, will check that when i have time

[andrepereiradasilva]

@ggppdk i'm now looking with tiume to your PR description.

you are making this:

JFactory::getDocument->addStyleSheetVersion('my.css', $version='3.1.1');
JFactory::getDocument->addStyleSheetVersion('my.js', $version='3.1.1');

when it should be this:

JFactory::getDocument()->addStyleSheetVersion('my.css', '3.1.1');
JFactory::getDocument()->addScriptVersion('my.js', '3.1.1');

But yes, the issue exists

[andrepereiradasilva]

@ggppdk please test/review #12836

[jeckodevelopment]

Closing since we have a PR to test for it.
Thanks

[ggppdk]

@andrepereiradasilva

Using $version='3.1.1',

is irrelevant, it is about readability and about avoid bugs

• you are simpling declaring a variable while calling the method
• it does not change the type of the passed parameter, in both cases it is 'string'

I usually do not use this, when only having 2 or 3 parameters
but for more i do for 3+ parameters

• Only when the code is going to be executed "many" times, then it should be avoided

Otherwise the performance difference due to the redudant variable declaration is non-measurable,
and probably PHP compiler, will disgard the variable declaration it completely

Example of readability, and avoiding mistakes:

$send_result = JFactory::getMailer()->sendMail(
    $_from = $data['mailfrom'], $_fromName= $data['fromname'], $_recipient = array($email),
    $_emailSubject, $_emailBody,
    $_html_mode=true, $_cc=null, $_bcc=null, $_attachment=null,
    $_replyto='some@sommmmmm.org', $_replytoname=null
);

You could of course declare the variables above the method call, but this means a few extra lines

[andrepereiradasilva]

sure please test the PR

[ggppdk]

doing already, thanks !