Library improvements

[lcobucci]

trafficstars

Great job with the library, I tested it yesterday and got quite impressed with the results! We'll start using this lib on a project and there are some modernisations and design decisions that we'd like to propose that will make it even better.

My ideias are (for now):

• [x] require PHP 7.1+ so that we can rely on proper type declaration and simplify the code (#142)
• [x] remove unnecessary polyfills (once the project requires PHP 7.1+) (#143)
• [x] use PSR-4 instead of PSR-0 to simplify the structure a bit (#143)
• [x] use Travis-CI build stages (#143)
• [x] add PHPCS for coding style (on build) (#143)
• [x] upgrade PHPUnit to 6.5 (#144)
• [x] add PHPStan for static analysis (on build) (#144)
• [x] use infection/infection to check for uncovered/escaped testing mutations (#144)
• [x] upgrade to Amp v2 (7e092b439bf3f871bb2b6d208ac1de0ad9d264ff)
• [x] implement functional tests (#147)
• [x] use callable instead of Closure to handle callbacks (#147)
• [x] add type declarations everywhere (#160, #168)
• [x] be more explicit regarding functions from global namespace (using \ or use function) - is a micro optimisation but an important one for this kind of lib (#168)
• [ ] make logger mandatory and only on the outer-most layer (less method calls and configuration)
• [ ] don't rely on singletons (injecting dependencies is a much better option)
• [ ] remove logger trait (simply rely on logger to), which will clean up the objects' interface
• [ ] allow persistent connections
• [ ] disconnect consumer before stopping the process - maybe related to #120? (#147)
• [ ] remove magic method __call() from config to make things more explicit
• [ ] add performance tests with PHPBench
• [ ] be more explicit regarding extension points (mark classes that SHOULDN'T be extended as final)
• [ ] drop support for pseudo-sync behaviour
• [ ] add support for https://github.com/reactphp too

Of course this is a big redesign of your package and takes a lot of work, but I'm whiling to help you if you're open for this kind of change.

What are your thoughts?

[nmred]

Hi, @lcobucci

Thanks for your advice，this Advice is very good, but my energy is limited, I'm afraid a short time can not be completed, but I welcome any help, to work together to improve this library

[lcobucci]

@nmred don't worry my idea is to send PRs, I just wanted to check upfront because some items might lead to big changes to the library and I wasn't sure if you were open for this kind of contribution =)

[nmred]

@lcobucci It is possible to be compatible when any change. And I will welcome to submit this kind of PR

[lcobucci]

@nmred awesome I'm a bit overwhelmed with maintaining @doctrine stuff now but I'll do this ASAP

[simPod]

What about not so general namespace?

[brstgt]

I also appreciate all work that has been done on that library. I also give a +1 to all the suggestions mentioned above. I'd even go one step ahead:

• Drop support for pseudo-sync behaviour.
• Using AMP in a sync environment is not efficient. For environments that don't run on event loops it will be probably better to go with a client like that: https://github.com/arnaud-lb/php-rdkafka
• Will produce better code, decoupled from the AMP loop state. IF someone still wants to use this in a sync way, he still can use AMP and run wait($promise)
• Introduce a new major version, break backward compatibility if necessary. This will make the refactoring and finding a stable interface much easier
• Producer should leverage promises instead of callback functions. Thats what promises are made for. Callbacks are somewhat 2005

[php-cpm]

this lib needs a lot of work to do to move on.