Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PoC][WiP] callables #39

Closed
wants to merge 4 commits into from

Conversation

schnittstabil
Copy link
Contributor

@schnittstabil schnittstabil commented Nov 24, 2016

This is not a serious PR, it is only a proof of concept – I've just found some time to investigate the idea of describing middlewares and delegates by callable and interface and what we might lose.

I hope this stuff will help to pursue the debate about that topic, so everybody can rethink his stand based on a more concrete level.

If you just chimed in

#37 is about the following change:

interface DelegateInterface
{
-    public function process(RequestInterface $request);
+    public function __invoke(RequestInterface $request);
}

interface ServerMiddlewareInterface
{
-    public function process(ServerRequestInterface $request, DelegateInterface $delegate);
+    public function __invoke(ServerRequestInterface $request, callable $delegate);
}

I know, we are discussing the process to __invoke change only for the DelegateInterface at #37 yet, if that bothers you, then just read over the ServerMiddlewareInterface::__invoke parts.

The idea: The interfaces are used to describe the signature and the behavior which the callables must fulfill. But moreover, the interfaces MAY be used by implementers, hence they get code completion and static type analysis, at least to some extend.

Possible Drawbacks

BCs and migration problems about __invoke were discussed in passionate debates. The main problem: Frameworks may already have introduced interfaces with __invoke but with different signature/contract, notably Zend\Stratigility.

Two ideas alleviate that problem:

  1. as we switch to callables, implementers can create a new PSR compatible middleware method:
class Middleware implements \Some\Other\MiddlewareInterface
{
    // legacy middleware
    public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $out = null) {…}

    // PSR middleware 
    public function process(ServerRequestInterface $request, DelegateInterface $delegate) {…}
}

// and use it:
$middleware = new Middleware();
$app->pipe('/', [$middleware, 'process']);
  1. Frameworks wich suffer from BC issues may check against the signature at run-time.

About this PR

Personally, I wasn't sure if type checking at run-time is powerful enough at PHP 5.4+ for our purposes.
So I gave it a shot and created two functions:

namespace Interop\Http\Middleware;

/**
 * Verify that the contents of a variable can be called as a delegate.
 *
 * @param mixed $var The value to check
 *
 * @return bool Returns TRUE if var is a delegate, FALSE otherwise.
 * @throws ReflectionException if the function does not exist.
 */
function is_delegate($var) {…}

/**
 * Verify that the contents of a variable can be called as a middleware.
 *
 * @param mixed $var The value to check
 *
 * @return bool Returns TRUE if var is a middleware, FALSE otherwise.
 * @throws ReflectionException if the function does not exist.
 */
function is_server_middleware($var) {…}

I believe they are thoroughly tested: Build Status I have written many delegate and middleware implementations only to document which delegates/middlewares are considered as valid and which are not. You may scan over the tests/Fixtures/*/{Valid,Invalid} directories – in my opinion they all work as expected.

The most interesting observations I've made:

I'm willing to transfer this stuff to http-interop and waive all copyright rights, so frameworks can safely use it or could create their own implementations based on this work. However, I would suggest to create a new project for that: PHP may eventually introduce new types of callables and after all it is still in an early stage…

Whether we chose the callables+interface style for middlewares or delegates or both or not at all, I'll be fine with it – As you perhaps know, the contract (the naming and the descriptions in the comments) is much more important to me…

Best regards
Michael

@schnittstabil schnittstabil mentioned this pull request Nov 26, 2016
@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 26, 2016

💣 EXPERIMENTAL – callables+interface PSR-15 implementions 💣

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 26, 2016

TODO: Reconsider client and server middlewares/delegates, more precisely:

interface DelegateInterface
{
    public function process(RequestInterface $request);
}

interface ServerDelegateInterface
{
    public function process(ServerRequestInterface $request);
}

Possibly related resources:

  • PHP RFC: Generic Types and Functions (Status: Draft)
  • PHP RFC: Union Types (Status: Declined – why?)
  • JavaScript Proxy enforces invariants:

    If the target object is not extensible, [[GetPrototypeOf]] applied to the proxy object must return the same value as [[GetPrototypeOf]] applied to the proxy object’s target object.

    • we may add an equivalent restriction to the middleware contract (interface comment) about calling $delegate with RequestInterface and ServerRequestInterface instances
    • we may allow a middleware container to throw exceptions if its delegate is called with a wrong instance

Done, see #40

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 27, 2016

Aura.Router already supports callables+interface PSR-15

Let's take a look at their Getting Started:

use Aura\Router\RouterContainer;

$routerContainer = new RouterContainer();
$map = $routerContainer->getMap();

// add a route
$map->get('blog.read', '/blog/{id}', function ($request, $response) {
    // …
    return $response;
});

// do the matching:
$matcher = $routerContainer->getMatcher();
$route = $matcher->match($request);

//… do some Aura specific stuff … 

// dispatch:
$callable = $route->handler;
$response = $callable($request);

I've thoroughly discussed Middleware Stacks at #35 and showed that Middleware Stacks can act as delegates – I know, some of you are sick and tired of this.

But with callables+interface Aura.Router instantaneously supports PSR-15 delegates:

// add a route
$map->get('blog.read', '/blog/{id}', function (RequestInterface $request) {
    // …
    return $response;
});

$map->delete('blog.delete', '/blog/{id}', new BlogDeleteMiddlewareStack());

Furthermore, we only need three lines to wrap the dispatching stuff and get a reusable middleware delegate:

$delegate = function (RequestInterface $request) use ($matcher) { // 1
    $route = $matcher->match($request);

    //… do some Aura specific stuff … 

    // dispatch:
    return $route->handler($request); // 2
}; // 3

Neither we nor they need to create a Proxy/Adapter classit just works!

If we stick with interfaces-only we will get myriad composer packages, which are essentially PSR-15 adapters. I have seen that before with Grunt plugins and Gulp plugins in the Node.js world, and I see all the Symfony2 Bundles out there. Just because they program to interfaces, doesn't mean they don't program to implementations: if the adaptee does a major version bump, the adapter/plugin/bundle package must do the same. And in the meantime many consumers have to wait – sometimes in vain.

In the Node.js the following already happens today: We got sick and tired of waiting – we started creating packages that are useful without Grunt and Gulp, and use them in our package.json scripts (essentially the same as composer.json scripts) and just use them everywhere we need the functionality.

I foresee the same with interfaces-only style: Either a new pattern/standard comes around the corner and everybody moves to it – and the vicious adapter circle begins again – or we remove the need to write adapters for many simple use cases at all.

Btw, @oscarotero made an awesome PSR-15 middleware which uses Aura.Router – he added some nice features to increase the usability within the context of middlewares, thus it is not one of those dump adapters…

@shadowhand
Copy link
Collaborator

There's way too much going on this PR. If the goal is to change the middleware signature to use a callable, that is one thing. The rest of this is out of scope.

Also, I am not sure what the point of DelegateInterface would be if the type hint is callable. It serves no purpose at that point, since the value of having a strongly typed signature is gone.

build/
vendor/
composer.lock
composer.phar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all of this being added? http-middleware contains only interfaces and will never be testable, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is not meant to get merged:

This is not a serious PR, it is only a proof of concept – I've just found some time to investigate the idea of describing middlewares and delegates by callable and interface and what we might lose.

I'm willing to transfer this stuff to http-interop and waive all copyright rights, so frameworks can safely use it or could create their own implementations based on this work. However, I would suggest to create a new project for that…

It was the easiest way to fiddle around and it allows us to discuss that here. If you think it would help, then I will create a new PR, only with the delegate stuff...

@@ -0,0 +1,17 @@
<phpunit bootstrap="./vendor/autoload.php" colors="true">
<testsuites>
<testsuite name="Zend\\Stratigility Tests">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕

@@ -8,11 +8,11 @@
interface DelegateInterface
{
/**
* Dispatch the next available middleware and return the response.
* Process a request and return the response.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commentary is still not correct. A delegate is deferring processing to the next available middleware. It is not, in and of itself, processing anything.

@@ -9,12 +9,12 @@
{
/**
* Process an incoming server request and return a response, optionally delegating
* to the next middleware component to create the response.
* the response utilizing $delegate.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't delegate the response.

use ReflectionFunctionAbstract;
use TRex\Reflection\CallableReflection;

if (! function_exists('Interop\Http\Middleware\is_delegate')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary. The only time we needed to use function_exists before defining functions is when namespaces were not available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, function_exists would have been necessary. See for example nikic/FastRoute#39

@@ -20,16 +20,49 @@
],
"require": {
"php": ">=5.3.0",
"psr/http-message": "^1.0"
"psr/http-message": "^1.0",
"raphhh/trex-reflection": "^0.2.3"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in favor of adding a global dependency on a reflection proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do it manually, but this would mean to create a function or class somewhere – or it would not get DRY. I'm not aware of any other way to share code which wouldn't get public 😒

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 27, 2016

Also, I am not sure what the point of DelegateInterface would be if the type hint is callable. It serves no purpose at that point, since the value of having a strongly typed signature is gone.

For ServerMiddlewareInterface implementors:

/**
 * @param DelegateInterface|callable $delegate
 */

I rarely use full featured IDEs, but the last time I've used them, I was able to navigate to the DelegateInterface by clicking on it, hence I can read how $delegate should be used:

  • How many arguments must be provided?
  • What types must the arguments have?
  • What will I get in return?
  • Must/should I catch errors?

In summary: I can read its contract.

Furthermore, there are some developers who cannot write code without their beloved IDE and will moan if they don't have any kind of IDE support 😉

For middleware container implementors:
I may want to implement to an interface rather than dealing with functions, e.g. Equip\Dispatch\Delegate:

class Delegate implements DelegateInterface
{
    //…
    /**
     * {@inheritDoc}
     */
    public function __invoke(RequestInterface $request)
    // …
}

Because I use the implements DelegateInterface and {@inheritDoc}:

  • I cannot remove the RequestInterface typehint or change it to a sub-type.
  • I can refer to the already existing DelegateInterface contract.
  • My generated API documentation will clearly state that this class can be used as delegate.
  • is_delegate do not need reflection => performance improvement for libraries which want to use it.

In summary: I can statically proof that this class will provide the right typing. And I can document that this class fulfills the delegate contract and it can be used to call any ServerMiddlewareInterface::process.

For the PSR-15 and its meta documentation:
In my opinion, it would simplify some of the writing:

  • the PHP static and run-time semantics of a class which implements such an interface is already given by PHP itself.
  • we can refer to that, i.e. the callable must behave in the same way as DelegateInterface::__invoke does.

@DaGhostman
Copy link
Contributor

-1 for the proposal from me (will give more detailed context later today), on top of my head:

  • callable is bad (keep in mind PHP is not JS)
  • I remember issues when mocking (with prophecy) and trying to test __invokable's response/result
  • I am annoyed beyond comprehension from the callable discussions, it was on the table for quite a while iirc and the decision taken was in favor of not using it. It hides what are you actually using, it allows for the use of anonymous functions/closures which are hell to debug in a larger application.
  • A few extra keystrokes will not be such a major benefit (keep in mind IDEs like PHPStorm have code completion: ctrl+space on a interface typehinted argument, boom your method is there)

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 28, 2016

@DaGhostman Probably, I've written to much and some points were drowned in this flood of information:


DelegateInterface and ServerMiddlewareInterface won't go away, you can still use them:

unicorn-middleware


keep in mind IDEs like PHPStorm have code completion: ctrl+space on a interface typehinted argument, boom your method is there

PHPStorm might be the very best PHP-IDE in the whole universe – But I still don't need it, it takes hours to load and encourages bad coding. But that is my personal opinion and I don't try to convert you to become better programmer – Why do you try to force me to use your coding patterns? I don't like them!


A few extra keystrokes will not be such a major benefit

This is not about those ->process keystrokes, it is about readability and reusability:

The LOC does not bother me much – The issue is: the amount of classes, the code needed to inject dependencies into them, and the adapters (e.g. see Equip\Dispatch\MiddlewarePipe) which must be written by almost every single developer to reuse those process middlewares.


it allows for the use of anonymous functions/closures which are hell to debug

That is your opinion and depends on your tools. I use different tools, TDD/BDD and CI for example – I haven't used debuggers for years and I don't miss them at all. Anyway, do you think debugging 3 different classes at 3 different locations is easier than debugging 1 single class?


I remember issues when mocking (with prophecy) and trying to test __invokable's response/result

If you mock an __invoke interface to get a callable test double, then you do it wrong:

public function test()
{
   $middleware = …;
   $request = …;
   $count = 0;

   $response = $middleware($request, function (RequestInterface $res) use (&$count) {
      $count++;
      $this->assertSame(…, …);
      return (new Response())->with…;
   });

   $this->assertSame(1, $count);
}

Furthermore, If you must use mocks and stubs to test your code, then your design is flawed: you have written code which is not reusable. In my example above, I don't need a mocking library, the callable is a valid delegate, which could be reused some where else.


It hides what are you actually using

I really hope I misunderstood you. Experienced developers call that Information hiding, program to interfaces, not implementations and especially Loose coupling:

a loosely coupled system is one in which each of its components has, or makes use of, little or no knowledge of the definitions of other separate components.

Hence, it is a feature – not a bug.


callable is bad (keep in mind PHP is not JS)

That doesn't help much. I don't remember when callable was introduced, but anonymous functions are available since PHP 5.3.0. Get used to them, they won't go away.

And if you don't want them, then don't use them, just stick with the interfaces
– I won't force you to write anonymous functions :bowtie:

@DaGhostman
Copy link
Contributor

DaGhostman commented Nov 28, 2016

@schnittstabil, with the dual type-hint in the comment or type-hinting against callable, you successfully achieved making the interface pointless. When you accept a callable you can get whatever, nor you can enforce the arguments of said callable, unless you type-hint to the interface, which defeats the purpose of using callable in first place.

On the IDE/Editors part, sorry but unless you are writing your code with pen and paper, every CODE editor I know/have used is binding ctrl+space to trigger code completion (never got too serious in vi/emacs).


The argument about code being written by everyone is kind of pointless, imo. Why do we always have to reinvent the wheel not everyone will have to rewrite it(I don't see the point personally but that is beyond the point of this discussion), like many (including myself) are using zendframework/diactoros for the PSR7 stack, why cant we use a simple middleware pipe (or whatever) to just get things done, there isn't much in the dispatching logic that can't be achieved without having 20+ middleware pipe implementation, we are fragmented enough as a community at the moment(in terms of packages/frameworks), which is probably one of the main problems/drags with this PSR passing.

Because symfony did not implement PSR7 in 3.x, iirc the controversy about PSR7 was still going on, now laravel/lumen use that, cuz... reasons and that PSR lacks adoption, whereas it clearly is a step in the right direction.


Well I will not agree fully on the mocking part, excuse me, but if you want to get a mock out of a interface - since you have type-hinted against said interface - you kinda have to (I avoid using callable as much as possible). Why I dislike callables, they tend to screw up DBC, you have no way to know what that particular function will expect (and since PHP7 is a thing) no way to enforce what it returns. I dislike having endless try-catch for a single function call, just to ensure that code will not get abused at some later point by me, not remembering what I was intending to do or die and the coworker picking up..


idk, why do you refer to the debugging as using an actual debugger, since

Debugging is the process of finding and resolving of defects that prevent correct operation of computer software or a system.

See also Debugging Techniques

I most of the time like to have a meaningful error messages, with a more informative trace, than a series of Object(Closure) that get me digging through the sources. And TDD/BDD are not debugging, you test for bugs, but often you may overlook and/or not think of an edge case that someone encounters, so you write tests to verify and fix and ensure it stays fixed, but in order to fix the bug you kinda have to know where it is, where to look


The point about information hiding, is not exactly on point in this context since this is a PSR, which will become a standard, which will not change, hence different implementations should behave the same way. SRP between the delegate and the middleware is preserved, so the middleware must not care about the delegate as long as the interface contract is truthful.

Again loose coupling, is still not applicable, since this is a standard, it is not likely to change in the near future. You are looking to decouple what from what exactly? Remove the contracts?


callable is bad (keep in mind PHP is not JS)
That doesn't help much. I don't remember when callable was introduced, but anonymous functions are available since PHP 5.3.0. Get used to them, they won't go away.

And if you don't want them, then don't use them, just stick with the interfaces
– I won't force you to write callables :bowtie:

Well, I've picked up PHP from around 4->5 era and got serious with it in the early 5.3 days. All I am saying is that, callable is too broad as a type, see my first point about how you defeat the purpose of the interface (one will care if they use closures). I would like to mention that the PSRs, FIG & PHP itself are looking more and more at OOP, using function all over the place will, make it too JS-like, which is not to say JS is a bad language, but it is different than PHP and making them similar is not necessarily a good thing.

Also I can't think of any other OO language that uses a lot of functions or any at all, except for JS, which imo is the reason everyone is so hyped about TypeScript, since it brings classes, interfaces, types, etc. to the table.

P.S
On the side, I would like to mention that it will be good if you could act a bit more professional and keep the guru attitude a bit lower, you are not always right, nor am I.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 29, 2016

@DaGhostman I'm sorry, I will try to improve.

On the one hand, you said that (almost?) everybody uses a code editor which provides code completion, and on the other hand you said type-hinting against callable makes the interfaces useless. But my screencast (hopefully) shows that in such IDEs you can open and read the contract of the interfaces and I've mentioned already other advantages of providing them just one comment above your first comment in this issue.

About your type-checking concerns:

  1. We cannot force the statically type-checking of the return type, because PSR-15 targets PHP 5.6 as we all know
  2. If you want to write code, which allows statically type-checking the return, then you still can: just put a return type after your __invoke(…):ResponseInterface or function(…):ResponseInterface and PHP will help you – nothing has changed.
  3. PSR-15 would guarantee, that your middleware will be called with the right callable, see Callable delegates #37 (comment) for a more detailed argumentation – and the interface phpdoc should state that as well (atm it does not)
  4. And if you still think that this is not enough, you can type-check the callables at run-time. I've written at my very first comment in this issue, why I believe that would be sufficient, and the whole PR is about proofing that.

About your PHP fragmentation argument: If PSR is not about interoperability, why should we standardize middlewares at all? I mean, is this only about all the big players? Should PSR-15 for middleware (stack/pipe/…) implmentors be less useful, because Symfony/Zend/Laravel/… would benefit from the lack of competition?

Maybe you could clarify the following, I don't really understand what you mean:

Well I will not agree fully on the mocking part, excuse me, but if you want to get a mock out of a interface - since you have type-hinted against said interface - you kinda have to (I avoid using callable as much as possible).

I don't want to dive into a debate about how to write good code or similar. I just want to clarify my statements above. I test first – always – that is, if I encounter a bug, the very first thing I do is writing a test to reconstruct the problem, thus TDD/BDD is my way to debug and I do not need phpdbg or an IDE for that. And I'm not alone, many developers have made the same experience.

I agree, that this is a standard and should be fully dedicated to the needs of the middleware context. But I don't understand why you all want to stick so much on this contract: this interface represents a delegate of a middleware container, and it is useless in any other context of middlewares. I have presented my point of view at #35. I don't say: lets reuse the DelegateInterface in some other context as well, e.g. like PSR-14 or similar – that would be very odd. Furthermore, we already use PSR-7 ServerRequestInterface within this PSR, but we could also decide to invent a new, more specialized or generalized RequestInterface only for this standard, just because there are also middlewares in asset pipelines etc. We are free to chose how general the interfaces are, hence I see a real benefit in having callable or __invokeable.

About functions in other languages: functional programming languages are gaining more and more ground (JavaScript is one of them) these days. But also all big OOP languages introduce more and more functional programming concepts: C#, Java and even C++! And I foresee that PHP will do the same. I believe, that sticking too much at OOP-only fails to appreciate current development trends.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 29, 2016

I would like to present a further argument. Lets take look at the ideas of most big players, I'm aware of. They all came up with modern, lean micro frameworks:

// Slim Framework Team: Slim
$app->get('/hello/{name}', function ($request, $response, $args) {…});

// Sensiolabs: Silex
$app->get('/hello/{name}', function ($name) use($app) {…}); 

// Laravel: Lumen
$app->get('user/{id}', function ($id) {…});

// Zend: Expressive
$app->get('/path', function (ServerRequestInterface $request, ResponseInterface $response, callable $next) {…});

Lets take a more general look at them:

  • They all use anonymous functions, thus they need to type-hint against callable or mixed.
  • All of those anonymous functions are essentially middlewares, or at least can be considered as delegates of middlewares (see MiddlewarePipe vs. MiddlewareStack #35).
  • Furthermore, they all MUST write dump adapters for ::process(), where they put their anonymous functions into MiddlewareInterfaces or DelegateInterfaces.

I try to find some time today, to take a deeper look at your anonymous functions produce hard to read stack traces argument, but PSR-15 targets all of these frameworks and all of them encourage the usage of anonymous functions, thus I do not believe it is a real problem.

The only, almost valid argument against __invoke (despite maybe your stack-trace one), I have seen so far was presented by @weierophinney in a hot, passionate debate about backward compatibility. The reason he feels so strongly about this:

If we define DelegateInterface such that it defines __invoke() instead of process(), we run into issues with existing double-pass middleware.

This means, the Zend guys decided to use an __invoke() styled MiddlewareInterface and having another one would introduce BC issue. As far as I can see, they do not consider __invoke() to be such bad thing at all – I believe they see __invoke() interfaces as the the most natural choice.

The suggestion, I present in this PR hopefully shows that BC issues can handled by the PSR-15 contract and run-time checks. Furthermore we would gain flexibility and resolve every, I mean really every possible name conflict with already existing process, disptach and __invoke interfaces.

I don't know, which arguments against __invoke+callables were presented somewhere else, it would be nice if anybody can give me some link to the most valid ones. But, my suggested approach seemed so promising to me, that we can support both programming styles, so I strongly believe it is worth to discuss it (again!?).

@DaGhostman
Copy link
Contributor

I think this arguments is mostly about preference, code style, et al, hence making it inappropriate on the PR. That being said, I am kinda against using the $app as god object. Lumen (use it at work) tends to do that a lot more than it should and that is acceptable (+ facades, which are singletons, which are antipattern, which I actually hate). The only candidate I would actually consider worthy of a mention is Zend Expressive, just because it allows the definition to happen in a configuration (limited to zf2 router?).

That said, I find myself advocating a lot lately that everything is middleware (tm), even controllers, enforcing OOP that (as my understanding goes) is that when applying composition you end up with how zf expressive uses it's route definitions/configs.

My point against type-hinting against callable is that: You cannot enforce anything when you use callable, a valid callable syntax is:

  • Class that has __invoke defined
  • closure
  • Array with class name and method name: ['ClassName', 'method']
  • String of class name and static method: ClassName::method

If that is the case and the interface is defined in such a way to allow that will allow bad practices (in my understanding). In some time PHP7 will become mainstream (a lot of companies start adopting it) and we will end up having to a) make a new standard because this one will not be enforceable; or b) it will no be used (defeats the purpose of it); or c) Majority will not actually adopt type hints and we will still have spaghetti written (think PHP4-style code, in PHP5, which became an issue with removing PHP4 constructors in 7 as a lot of projects ware using them and will slow adoption, hence deprecated and will have to drag them till next major version, which is god knows how far ahead).

A framework(lets all agree that we are on the same page and we are talking about OOP frameworks, regardles nano, micro or otherwise) imho should imply/enforce good practices and if you and I, as experienced developers will comply with the interface, many newer devs will not, since the new hype is using closures instead of classes, project grows and things tend to go sideways in terms of SOLID. Those closures will - at some point - need to become classes, since they have grown to 20-30 lines and helper methods should be used applied to keep things readable O. The D also goes out the window, since there is no concept of DI in closures, yes I am aware of use, but is that really the best way to go, don't think so + that is not DI you are referencing an instance (all code have access to that variable) also you pretty much invent singletons. One might reference the container you say? Well, say hello to Service Locator, which breaks S. Come to think of it, callable itself violates the I - not an interface, if there is an interface that goes for __invoke that makes code confusing if you are a newcomer or haven't worked with the framework. To expand my point even further, when using closures for a lot of things, we are pretty much heading to the JS callback hell, since we already have DI containers (Yii2, pimple - on top of my head) that allow for closures to be used instead classes, so imagine, there you have a callback hell, maybe not as severe as in JS, but let that for people to decide.

In my opinion this is a reason enough to keep away from it, I will prefer any descriptive code, which at first glimpse will give me a rough idea what it is doing, rather than code that will end up looking like clojure/lisp (i picked PHP you know).

I am rising my concerns since I believe that the idea behind PSRs i good, not always likable at first (think PSR7 no likable), but on the long run it pays. There should be a discussion as this is not something to be taken lightly. Now the hype is to lambdas, I imagine soon (if not already) having arrow functions, like in JS, since they have grown popular in JSAnd we have the proposal already, but they ware present in Python for a while now and not that many people cared, why now?


Sorry I couldn't help, but post this. Kinda illustrates my point and it exists, so it is not such an imaginary problem

So this...

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Nov 29, 2016

As long as we talk about fat frameworks, I fully agree: You want SOLID, classes and all that stuff. But those fat frameworks are really, really difficult to learn for novices, with all the conventions and configuration files; and additionally most of them are hard to setup for them. But most worst: a good pattern in one framework is considered bad practice in the next one.

Personally, I want to start with a simple solution and if time comes I want to switch smoothly to a more powerful one – and middlewares and delegates are perfect for that:

// kickoff (contact.php)
$stack = new Stack(function (ServerRequestInterface $request, callable $next) {
   // send email…
});
$stack->push(…);
$stack(new ServerRequest());

// Sometimes later, I can switch to a mirco framework – and even later,
// when I need DI and all that stuff, I can switch to a fat framework
class ContactPostAction implements ServerMiddlwareInterface {…}
// and even later, you may want to reuse your `ContactPostAction` company-wide with:
class BasicMiddlewarePipe implements ServerMiddlwareInterface {
   private $pipe;

   public function __construct(ServerMiddlwareInterface $action)
   {
      $this->pipe = new MiddlewarePipe([
          new Middlewares\CsrfProtection(),
          // whatever…
          $action
      ]);
   }

   public function __invoke(ServerRequestInterface $request, callable $next)
   {
      return ($this->pipe)($request, $next);
   }
}

// composer require foo-company/contact-action foo-company/basic-middleware-pipe

// and just use it in a micro-framework/fat framework or standalone:
$app->get('/contact', new BasicMiddlewarePipe(new ContactPostAction()));

Take that contact example: in many, many use cases you don't need DI, caching and whatever: neither today nor later – micro frameworks exist exactly because of that: they are fast and easy to set up: just create a single index.php.

tl;tr If we do not have callables or at least __invoke we have to write adapter/proxy stuff everywhere.

You may say, it is bad practice anyway – I say, an anonymous function fits best in such simple use cases.
With the words of a Turing Award winner:

Premature optimization is the root of all evil – Donald Knuth


@DaGhostman I would thank you, that you gave me more details about your type-hint to callable concerns. Furthermore, I hope you would give me feedback about this improved version too:

Definition: A middleware is either,

  1. A class which implements ServerMiddlware5Interface
    and fulfills its contract (type-hints and phpdoc)
  2. A class which implements ServerMiddlwareInterface
    and fulfills its contract (type-hints and phpdoc)
  3. A callable with a signature like ServerMiddlware5Interface::__invoke or ServerMiddlwareInterface::__invoke which fulfills the contract (phpdoc) resp.

The interfaces would look like:

interface DelegateInterface
{
    /**
     * Dispatch the next available middleware and return the response.
     *
     * @param RequestInterface $request
     *
     * @return ResponseInterface
     */
    public function __invoke(RequestInterface $request):ResponseInterface;
}

interface ServerMiddlewareInterface
{
    /**
     * Process an incoming server request and return a response, optionally delegating
     * to the next middleware component to create the response.
     *
     * @param ServerRequestInterface $request
     * @param DelegateInterface      $delegate
     *
     * @return ResponseInterface
     */
    public function __invoke(ServerRequestInterface $request, DelegateInterface $delegate):ResponseInterface;
}

interface Delegate5Interface
{
    /**
     * Dispatch the next available middleware and return the response.
     *
     * @param RequestInterface $request
     *
     * @return ResponseInterface
     */
    public function __invoke(RequestInterface $request);
}

interface ServerMiddleware5Interface
{
    /**
     * Process an incoming server request and return a response, optionally delegating
     * the request utilizing $delegate.
     *
     * @param ServerRequestInterface $request
     * @param Delegate5Interface     $delegate
     *
     * @return ResponseInterface
     */
    public function __invoke(ServerRequestInterface $request, Delegate5Interface $delegate);
}

Constraints:

  1. a PHP5+ middleware container MUST support middlewares of type 1., 2. and 3.
  2. a PHP7+ middleware container MUST support middlewares of type 2. and 3. and MAY/SHOULD support middlewares of type 1.

Of course, the constraints are debatable, especially the MUST/SHOULD/MAY/etc.


This would allow us to support many, many use cases from simple to complex ones.
And furthermore we get these advantages:

  1. A smooth migration path for anonymous functions to full featured frameworks
  2. A smooth migration path for Zend\Stratigility\MiddlewareInterface to type 2. via type 1.
  3. A smooth migration path for PHP5 to PHP7 middlewares
  4. PHP7 return type-hinting already today, with all the code completion stuff!

Please note: not only the 1. advantage relies on run-time checks, the 2. and 3. do as well.
Even containers which only want to support the interfaces would probably do:

if ($m instanceof ServerMiddlewareInterface || $m instanceof ServerMiddleware5Interface) {…}

EDIT: callable removed from ServerMiddleware5Interface.
Clarification: In my opinion, it is possible to additionally allow callable delegates – but that would lead to a very complicated design with many drawbacks.

EDIT2: Extended contact-usage example.

@mindplay-dk
Copy link
Contributor

I can't keep up with this thread, but this IDE hate I'm hearing? Stop that. Type-hints are not only about IDE support, they're about support for any static analysis tools, including offline QA tools like CodeSniffer and MessDetector. Yes, that also happens to give us IDE support. Your personal feelings about IDEs have no place in this thread.

@weierophinney
Copy link

This means, the Zend guys decided to use an __invoke() styled MiddlewareInterface and having another one would introduce BC issue. As far as I can see, they do not consider __invoke() to be such bad thing at all – I believe they see __invoke() interfaces as the the most natural choice.

You're making some rather broad assumptions that are, frankly, incorrect.

We added our MiddlewareInterface because several of our users insisted. I was reluctant, precisely because I knew that PSR-15, though not yet proposed, would be, and I didn't want to define something we might end up scrapping.

Additionally, the problem is not with the MiddlewareInterface, but rather with delegation. Zend\Stratigility\Next::__invoke() currently

  • Requires a request argument, typehinted against ServerRequestInterface
  • Requires a response argument, typehinted against ResponseInterface
  • Optionally allows an "error" argument

The problem with having DelegateInterface define __invoke is that, in order to support existing double-pass middleware side-by-side with http-interop middleware, we hit a few snags:

  • We have to change the typehint of the first argument. This is okay, as it's a loosening of type.
  • We have to make the second argument optional.

It's having the second argument optional that presents a problem. If http-interop middleware calls on the delegate, and passes only the request, but the next in the stack is double-pass middleware, how does it get the response? Additionally, we need to check the signature of the next middleware in the stack to see what type of middleware it is, in order to determine what arguments to pass to it.

These are all solvable, but they're far easier to solve when the methods differ.

Regarding having callable as a typehint for middleware, and using __invoke() in the specification, I have one argument I haven't raised previously, and it's about middleware composition and invocation.

If I compose one middleware inside of another:

class SomeMiddleware
{
    public function __invoke($request, $delegate)
    {
    }
}

class CompositeMiddleware
{
    private $nested;

    public function __construct(callable $middleware)
    {
        $this->nested = $middleware
    }

    public function __invoke($request, $delegate)
    {
         // call the nested middleware as part of our work...
    }
}

The above leads to some interesting gymnastics, based on the PHP version you're using.

Under PHP 5, you need to do the following:

$nested = $this->nested;
$response = $nested($request, $delegate);

and, in point of fact, that may not work if the nested middleware is a callable of the form StaticClass::method (which passes is_callable(), but cannot be invoked directly in that form until PHP 7) — which means you have to use call_user_func() to be completely certain you can invoke it properly.

Under PHP 7, you can do this:

$response = ($this->nested)($request, $delegate);

In every case, having a defined method, instead of relying on the callable typehint, eliminates the possibility of error:

$response = $this->nested->process($request, $delegate);

Not using callable for any of the typehints ensures that the above works, eliminates errors, simplifies invocation, and adds clarity.

@shadowhand shadowhand closed this Dec 6, 2016
@shadowhand
Copy link
Collaborator

I am closing this out, as per other issues/PRs on this topic. Callable type hint reduces runtime safety and static analysis.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Dec 6, 2016

You're making some rather broad assumptions that are, frankly, incorrect.

Sorry, for that. But many other implementers use callable or __invoke, and if I understand you correctly, then even your users insisted on such an interface. – Maybe they are all wrong, but in case of that, it needs a good argument for not choosing __invoke, which I have not seen yet.

that may not work if the nested middleware is a callable of the form StaticClass::method

@weierophinney Are you concerned about that 2 year old bug? https://bugs.php.net/bug.php?id=68475

Anyway, supporting both types of middlewares side-by-side by implementing both interfaces, is a design mistake in my opinion and will lead to hard to debug errors: #44 (comment)

@shadowhand Thanks for closing. I see that a PSR should not encourage callable middlewares. But, in my opinion, protecting developers from themselves by not choosing __invoke goes to far. Especially because #43 shows: many real world implementations must use lesser strict type-hinting than callables if we not choose __invoke.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Jan 17, 2017

FYI, comparing this PR with PHP 7.1 iterable there are only 2 differences:

  1. Instead of Traversable, array and is_iterable,
    I've suggested MiddlewareInterface, callable and is_middleware.
  2. We cannot have a middleware pseduo-type (like iterable) w/o writing some C code.

Well, the second point is clearly unfortunate. But I vehement protest against the validity of concerns @DaGhostman and others have mentioned, who believe that this approach does not fit well in the PHP world. It's obviously just the modern way how standards deal with missing language features (like typesafe callable).

In addition, PHP 7.1 iterable also refers to callable at the very beginning:

This RFC proposes a new iterable pseduo-type. This type is analogous to callable, accepting multiple types instead of one single type.

Thus, as far as I can see, if callable is sooo bad, then iterable would never have been implemented for the same reasons, instead everybody would just use Traversable interface – as we already did in PHP <7.1.

@shadowhand
Copy link
Collaborator

Comparing iterator and middleware is flawed. An iterator can (for the most part) be used as an array. This is, without question, not the same thing as a middleware being a callable.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Jan 17, 2017

An iterator can (for the most part) be used as an array.

Well, it seems that you do not often use Traversable, Iterator, Generator or any other Recursive Data Type – otherwise you would not have claimed that.

This is, without question, not the same thing as a middleware being a callable.

That's not the point and doesn't matter here. If you reread my #39 (comment), then you will see that I'm talking about the concerns about the impact of this solution on the PHP ecosystem (see the FIG Mission Statement, why this have to be considered).

@shadowhand
Copy link
Collaborator

The iterable comment you posted shows exactly why using __invoke (callable) for middleware is a bad idea:

This type is analogous to callable, accepting multiple types instead of one single type.

It allows accepting multiple types, rather than one strict type.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Jan 17, 2017

__invoke (callable) … allows accepting multiple types, rather than one strict type.

WUT? We had a similar debate about DelegateInterface some time ago and it is thanks to you @shadowhand, that we have made a substantive step forward and have realized that callable and __invoke differ in exactly this point (#37 (comment)):

it would not be possible to provide strict typing, because a generic callable cannot specify the return type. Type hinting against DelegateInterface ensures this:

class DelegateInterface
{
    public function __invoke(ServerRequestInterface $request): ResponseInterface
}

Exactly the same holds for middlewares, which is due to the fact, that this PSR clearly enforces:

Middleware using this standard MUST implement the following interface:

  • Psr\Http\ServerMiddleware\MiddlewareInterface

@schnittstabil
Copy link
Contributor Author

@shadowhand For clarification, if we consider the following interfaces:

interface DelegateInterface
{
    public function __invoke(ServerRequestInterface $request);
}

interface MiddlewareInterface
{
    public function __invoke(ServerRequestInterface $request, DelegateInterface $delegate);
}

Then callables don't become PSR middlewares:

$m = function(ServerRequestInterface $request, DelegateInterface $delegate) {…};

var_dump($m instanceof MiddlewareInterface); // => false

@schnittstabil schnittstabil mentioned this pull request Feb 3, 2017
3 tasks
@schnittstabil
Copy link
Contributor Author

schnittstabil commented Feb 4, 2017

@DaGhostman, @atanvarno69 and anybody else who's interested.

While this debate is already over, @DaGhostman's #34 (comment) showed me that there are open questions.

@DaGhostman None of the SOLID concerns you've mentioned (neither above nor anywhere else around here) make sense. If you still do not know Why, then please read up their definition and ask.

I'm willing to answer all of your questions.

@schnittstabil
Copy link
Contributor Author

schnittstabil commented Feb 4, 2017

Btw, I also have a question regarding to this:

My point against type-hinting against callable is that: You cannot enforce anything when you use callable, a valid callable syntax is:

  • Class that has __invoke defined
  • closure
  • Array with class name and method name: ['ClassName', 'method']
  • String of class name and static method: ClassName::method

@DaGhostman Is not the only one who believes that you cannot enforce anything. I mean, if I type-hint against callable then I enforce that I can use it as a (callable) function – that's all; and fully aligns with LSP and the type hierarchy etc:

function foo(callable $bar) {
    $bar();
}

Hence, why did @DaGhostman (and others) enumerate valid callables like a mantra? Do they just do not like the syntax and/or semantic of PHP?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants