@nikosdion @phproberto @ggppdk can you review this and check if there are no B/C issues now (and the introduced B/C issues in https://github.com/joomla/joomla-cms/pull/12028/files are now reverted).

mantainers can you add a release blocker tag here?

While this is not breaking b/c with Joomla 3.6.4 as far as I can see (more eyeballs on the code will be appreciated) I did spot that you have renamed a lot of method parameters (without changing their intent). Changing the parameter signature will cause code extending this class to emit notices. In fact a 3PD cannot solve that unless they create two different classes for each version of Joomla, causing either code duplication or inelegant code structure. It is therefore unadvisable to change the parameter names in a minor release.

I'd be happy with this PR if the original parameter names were preserved, e.g. retain public static function check($userId, $action, $asset = null, $preload = true) instead of public static function check($userId, $action, $assetKey = null, $preload = true). I understand that the parameter name $assetKey is better than $asset but we shouldn't touch that until 4.0 where there's a reasonable expectation of breaking b/c anyway.

(Edit: in case it wasn't very obvious from what I said: Good job!)

In fact a 3PD cannot solve that unless they create two different classes for each version of Joomla, causing either code duplication or inelegant code structure. It is therefore unadvisable to change the parameter names in a minor release.

Correct, but i think you meant to speak of type declaration (type hint) of the function's arguments
and in this case there is no type declaration for them

About
-- method's parameter names
-- and their default values too
I think, it is allowed to change both (or is it not allowed in some specific PHP version ?)

e.g. the following example, changes both function's argument names and their default values too
and it works without warnings or errors

class base_class
{
    function some_func($word=null)
    {
        return 'i am base: ' . $word;
    }
}

class extended_class extends base_class
{
    function some_func($text='hello')
    {
        return 'i am extended: '. $text;
    }
}

$test = new extended_class();
echo $test->some_func();

I believe in PHP 7 it does throw a notice? I can't check right now. Can you please do that?

I believe in PHP 7 it does throw a notice? I can't check right now. Can you please do that?

Ok, i have tested in PHP 7,
it does not throw any notices / warning with error_reporting to maximum or development

2 days ago i had the same thought and made similar comment for another PR

but after testing it seems that both renaming parameters and changing default values,
are both allowed in PHP 7 too, without any notices or warnings

Ah, good! I stand corrected. In this case I have no objections to this PR.

(Edit: in case it wasn't very obvious from what I said: Good job!)

Thanks!

So for what i understand, from your review you think there are no B/C issues. it would be good if someone else check that too, more eyes is always better.

Is also needed two tests in the behaviour itself to check all works fine.
@Fedik sorry to ask you again this, but can you test this new one?

@andrepereiradasilva no problem, it already was in my "to do" ?

I have tested this item ✅ successfully on 7b6b707

I have tested this item ✅ successfully on 7b6b707