chan-sccp / PAMI

Fork of PHP Asterisk Manager Interface ( AMI ) supports synchronous command ( action )/ responses and asynchronous events using the pattern observer-listener. Supports commands with responses with multiple events. Very suitable for development of operator consoles and / or asterisk / channels / peers monitoring through SOA, etc
http://marcelog.github.com/PAMI
Apache License 2.0
26 stars 21 forks source link

Broken backward compatibility in 2.0.3 #9

Closed alexpts closed 4 years ago

alexpts commented 4 years ago

In commit https://github.com/chan-sccp/PAMI/commit/ef992a0faeb2cd3037f40ac4c45c445715135979 was broken backward compatibility. I have bug after update to 2.0.4 from 2.0.1.

https://semver.org/

Problem related with class AttendedTransferAction. We can`t remove and move arguments to setter method in patch or minor version.

I suggest to revert signature of AttendedTransferAction. It is necessary for safe updates in one major version

dkgroot commented 4 years ago

@alexpts Hi Alex, Is the problem then you were using "AtxferAction" instead of AtrendedTransferAction ? If so we can use add a temporary/deprecated alian Action that for some time. Is that the problem you wanted to raise ?

So we keep using the new name, and just add an alias class that intercepts the call and redirects to the new implementation, Would that be acceptable ?

class_alias(AttendedTransferAction::class, AtxferAction::class);
alexpts commented 4 years ago

@dkgroot problem related with argument $context.

Old revision has 4 arguments public function __construct($channel, $extension, $context, $priority) New revision has only 2 arguments public function __construct($channel, $exten)

After update from 2.0.1 to 2.0.4 my code send $action = new AttendedTransferAction($transferChannel->channel, $phone, $context, 1);. Now argument $context ignored. It is problem.

I find it and can fix my code, but other people can has it problem too . Broken backward compatibility can only major version.

dkgroot commented 4 years ago

If you have the time, could you create a PR, that will re-introduce the old class and the new class. And add a deprecation warning to the old one. We should be fine. Currently not near a pc where i can test, so if you could, that would be perfect. Thanks !

dkgroot commented 4 years ago

Would this help to get around the changed constructor issue ?

    function __construct() {
        $argv = func_get_args();
        switch( func_num_args() ) {
            case 1:
                self::__construct1($argv[0]);
                break;
            case 2:
                self::__construct2( $argv[0], $argv[1] );
                break;
            case 3:
                self::__construct2( $argv[0], $argv[1], $argv[2] );
         }
    }
alexpts commented 4 years ago

Ok, I will prepare PR.

I don`t understand about new class. I can`t find new class AtxferAction in repository. I see only modification class AttendedTransferAction.

dkgroot commented 4 years ago

No the old name of the AttendedTransferAction used t be AtxferAction (I think). Thanks for prepping a PR !

dkgroot commented 4 years ago

Ok looks good. Do we still need the old AtxferAction class name ?

alexpts commented 4 years ago

Sorry, I don`t understand about old name AtxferAction. My problem related with 3th argument in AttendedTransferAction::__constructor. It fix help me

dkgroot commented 4 years ago

Accepted the PR :-) Thank you very much for creating this backward compatible solution. I will try to be more carefull in the future.

Closing this issue. Dropping the issue about the old name.