thephpleague / tactician

A small, flexible command bus
http://tactician.thephpleague.com
MIT License
858 stars 88 forks source link

Make Middleware::execute internal #74

Closed sagikazarmark closed 9 years ago

sagikazarmark commented 9 years ago

Nowadays I spend more time to use proper visibility everywhere, but it is not always easy.

For example in this case Middleware::execute must be public, since it is called from the outside world. However, it should not be called by the user. For these cases I would consider adding the @internal docblock.

While it is not the original purpose according to the phpDocumentor documentation, I've seen it many times (even in symfony), so I would say it is acceptable.

mikemix commented 9 years ago

That command is public as it belongs to the interface. As such in my opinion is public and should not be marked as internal.

sagikazarmark commented 9 years ago

It is public so it can be called by the commandbus. But a user should never call it.

mikemix commented 9 years ago

Commandbus is a user developer in this case.

sagikazarmark commented 9 years ago

Should I say developer then? But I see the point...

mikemix commented 9 years ago

@internal annotation is useful with public methods which really should be private, for example: entity setters for magical, hydrating purposes. With this annotation you encourage yourself not to use such methods.

Interface methods however are meant to be public always. That is the definition of an interface.

rosstuck commented 9 years ago

Technically, the internal annotation is meant to denote which part of the docs are for internal library developers. However, it is often used for methods that need to be public for language or performance reasons but shouldn't be called directly (such as callbacks, readable properties, etc).

Neither of those apply directly here and I'd like to encourage folks to call their middleware's execute methods: from their tests.

I've seen some hairy stuff with early command buses where people attached "decorators" at runtime dynamically but luckily, no one using tactician seems to have tried that yet. So, good impulse but I'll pass on this one for now. Thanks though! :smile: