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

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

@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: #11289, #12373, #12429, #11289 and #12373.
• For CSS: #12375, #12720, #12781, #12720 and #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.

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

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

np, will check that when i have time

@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

@ggppdk please test/review #12836

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

@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

sure please test the PR

doing already, thanks !