? Pending

User tests: Successful: Unsuccessful:

avatar philip-sorokin
philip-sorokin
14 Jan 2018

In legacy calls it is common to have the args as a string. So, if the first/second arg is not array, it should be proccessed as legacy.

avatar philip-sorokin philip-sorokin - open - 14 Jan 2018
avatar philip-sorokin philip-sorokin - change - 14 Jan 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Jan 2018
Category Libraries
avatar philip-sorokin philip-sorokin - change - 14 Jan 2018
Labels Added: ?
avatar philip-sorokin philip-sorokin - change - 14 Jan 2018
The description was changed
avatar philip-sorokin philip-sorokin - edited - 14 Jan 2018
avatar philip-sorokin philip-sorokin - change - 14 Jan 2018
The description was changed
avatar philip-sorokin philip-sorokin - edited - 14 Jan 2018
avatar Quy
Quy - comment - 14 Jan 2018

See #15641. The final decision was to fix the faulty code that called this method.

avatar philip-sorokin
philip-sorokin - comment - 14 Jan 2018

@Quy, what final decision? The PR in your example was closed.

avatar Quy
Quy - comment - 14 Jan 2018

To fix the faulty calling code and not the method. Read this comment.

avatar philip-sorokin
philip-sorokin - comment - 14 Jan 2018

@Quy, can you cite it here, please? I have no idea how this related to what I propose.

avatar Quy
Quy - comment - 14 Jan 2018

I fixed the link. Here is the comment:

I tend to say this PR isn't something we should do, as it is only a bandaid for faulty code in 3rd party extensions. It should be fixed in the respective extensions.

If we're going to do something like this, then it has to be deprecated right away with at least a log message. But then, the current warning is fine as the code still works and the extension developer gets notified of his faulty code right away. On productive servers, those warnings should be disabled anyway.

avatar philip-sorokin
philip-sorokin - comment - 14 Jan 2018

@Quy, what about b/c and coding standards "the code without warnings and notices"?

avatar Quy
Quy - comment - 14 Jan 2018

This PR is masking the issue where the warning origins because apparently there is code somewhere which calls this method wrong. Read the discussion in #15641.

avatar philip-sorokin
philip-sorokin - comment - 14 Jan 2018

@Quy, this PR is completelly different. It is soft check what type of arguments is passed. If any of arguments has wrong type, it is processed as legacy. In your example it is forced it to be a certain type.

avatar mbabker
mbabker - comment - 14 Jan 2018

At least with the stylesheet method, if the $options argument were typehinted, this would be akin to removing the typehint and allowing any type. Both at 3.6.5 and 3.7.0 where the signatures were changed, that argument was always documented to require an array so any code passing in anything other than an array is Doing it Wrong™.

For the script method, in 3.6.5 the second and third arguments were boolean then the refactoring made them arrays. Personally, I would change the API internals to throw an InvalidArgumentException if those arguments are not of those types. Because this is again code that is Doing it Wrong™.

It is not our role as the API providers to cater to developers who are doing it wrong. Based on our documentation, that is what we should be coding for and any other condition is invalid use of the API.

avatar brianteeman
brianteeman - comment - 16 Jan 2018

closed for the reasons stated above by @mbabker

avatar brianteeman brianteeman - change - 16 Jan 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-01-16 20:40:00
Closed_By brianteeman
avatar brianteeman brianteeman - close - 16 Jan 2018

Add a Comment

Login with GitHub to post a comment