http-interop / http-middleware

PSR-15 interfaces for HTTP Middleware
MIT License
73 stars 7 forks source link

Prepare for review phase #34

Closed shadowhand closed 7 years ago

shadowhand commented 7 years ago

While I realize there is still discussion around DelegateInterface (see #24), I feel this spec is complete. As such, we should begin preparing to submit this to FIG for the review phase. Doing so will require that:

shadowhand commented 7 years ago

Feel free to update the issue description @http-interop/http-middleware-contributors

schnittstabil commented 7 years ago

Aww, sorry @shadowhand, I wasn't aware you want to do that. I hope, I do not disturb the standardization process by my https://github.com/php-fig/fig-standards/pull/845 PR. To be honest, I don't know the exact bureaucratic procedures… :sweat:

shadowhand commented 7 years ago

@schnittstabil this is our "working" space, so that we can have a repo with a working implementation to find and resolve implementation problems. Once we are happy with the state of this repo, we will submit everything back upstream to FIG for review and (hopefully) acceptance.

schnittstabil commented 7 years ago

I see and I'm sorry.

schnittstabil commented 7 years ago

Working with http-interop/http-middleware sounds really cumbersome.

I think we should do instead:

  1. manage our own php-fig/fig-standards fork at http-interop/fig-standards
  2. create one or more working branches of our http-interop/fig-standards#master, e.g.:
    • http-interop/fig-standards#http-middleware
    • http-interop/fig-standards#http-factory
  3. everybody have to create PRs to http-interop/fig-standards#http-middleware
  4. once we are happy, we eventually squash and merge/rebase into our http-interop/fig-standards#master
  5. and create a PR from http-interop/fig-standards#master to php-fig/fig-standards#master

@http-interop/http-middleware-contributors What do you think?

alamin3000 commented 7 years ago

I honestly think current proposal has issues. And I am afraid that this design may not survive/accepted by community at large in the long run. The design may look right or sound cool at the moment... But I really do see some subtle but problematic issues.

I am trying to prepare and make this PSR less of impact for my team/company development anyway. I am designing middleware layer very low-level implementation (regardless of this interface or existing __invoke). Then create another layer visible to end-user-developers using Command Pattern. This is good practise anyway, but I have a feeling that writing code/action handlers how Slim + Expressive + others introduced will not be practised using this interface.

Anyway, from my short experience communicating with this "standard" community, I get a feelings that it is a decision of few over majority, just like everything else.

shadowhand commented 7 years ago

@alamin3000 I think it is safe to say that every PSR (save maybe PSR-0) has experienced significant pushback and ultimately understood to be the right way forward. We don't all have to like the standards, only recognize that they are the product of smart people dedicating a lot of time looking at the options and making the best possible choices.

As for Expressive, the next version has already implemented these interfaces and a large collection of middleware already exists using these interfaces. For the most part, the feedback from people who are actually using these interfaces is that they do work and are an improvement.

alamin3000 commented 7 years ago

For expressive, I am not too worried. You guys are cousins anyway.

I am sure all "smart" people are working on it. But this interface isn't about smartness. There isn't a single line of code. It is simply a decision making. Any experience developers can come up with 10 dozen interfaces, including this one. No one is saying this is a bad design decision. But the real concerns still remains: 1) We don't want middleware to have method name process, or any other arbitrary name for that matter. It should be __invoke or invoke to make it generic execution of code. Other names are part of other development patterns and honestly doesn't make sense for middleware. 2) Having same name for both delegate and middleware not ideal as mentioned by others. 3) Name of the interface for delegate is questionable/confusing, again mentioned by @schnittstabil 4) Double pass vs single pass arguments really doesn't hold too strong to simply change the entire interface. If only thing you guys wanted to have it strong typed, you should've done it with __invoke as well.

Anyway. I still don't think this is NOT ready for FIG. You smart guys should think this through just little more.

@shadowhand BTW, do you know how much time others (not part of the decision making) and myself has dedicated to this PSR so far?

moufmouf commented 7 years ago

@alamin3000 I think this issue was open by @shadowhand to prepare for the PHP-FIG review phase. You can certainly voice your opinion during the review phase and in the end, the important thing is that it is thoroughly discussed and documented.

@shadowhand , you certainly will want to write a "META" doc explaining why each decision was made (and possibly who was involved in the making).

Also, when working on container-interop, we had a section of the container-interop README highlighting the known implementations. You might consider doing the same here (to show there is some adoption).

A few tips about tracking the adoption:

One is not (yet) in the list above because I just published it to Packagist a few minutes ago: thecodingmachine/symfony-httpinterop-bridge (this is a bridge between PSR-15 and Symfony middlewares)

shadowhand commented 7 years ago

@alamin3000 to your points:

  1. That's extremely unlikely to change. The current README explains why a method name was chosen for middleware.
  2. This is still up for debate, see #37. I'm on the fence but leaning towards a callable $delegate implementation.
  3. Naming is hard and we could bikeshed about names all day every day. Delegation pattern is well understood and fits here.
  4. Again, the README explains why lambda was chosen. This has been debated to death and is extremely unlikely to change.

I appreciate the feedback and hope you will continue to provide it during the review phase. Just make sure you read all the documentation so that your arguments have context.


@moufmouf our current README is the meta doc, and it has a FAQ at the bottom about design decisions.

The packanalyst link is super interesting and helpful, thanks for sharing.

schnittstabil commented 7 years ago
  1. That's [renaming process] extremely unlikely to change. The current README explains why a method name was chosen for middleware.

@shadowhand You have to read more carefully – The README does not explain why the name process was chosen; it only tries to explain why the name is not __invoke.

schnittstabil commented 7 years ago

We don't all have to like the standards, only recognize that they are the product of smart people …

What an unfortunate arrogant statement – clearly unworthy of a PSR editor, who's responsible for coordinating contributors.

@jschreuder Thanks, it was not my intention to troll around. I've updated my comment to clarify that.

jschreuder commented 7 years ago

At this point you're only endlessly rehashing your previous arguments. And that last comment is plainly just trolling. Your behavior has gone way past what can be considered constructive in any way.

schnittstabil commented 7 years ago

@jschreuder I've never criticized that the README lacks that information, have I?

If you take a look at Container Meta Document - 7. Interface methods, then you see that this is a fair question:

The choice of which methods the interface would contain was made after a statistical analysis of existing containers. …

  • a large majority name such method get()

@mnapoli and others have chosen the name get because it is the most used one, and thus made clear WHY they propose get and not make or resolve – PSR-15 lacks that information.

DaGhostman commented 7 years ago

@schnittstabil actually, make and resolve don't make sense in the context of a container. the only alternative which conveys the action of the method is retrieve, but other implementations already have use the simplified version i.e get so why not stick with it? It makes sense. make will make sense for an interface for a factory, and resolve will in a service locator approach, which iirc is considered anti-pattern, so....

You are putting that statement out of context and/or do not take a look at the broader picture in a abstract way. Now, since your major point is to use callable and __invoke, they do not give any information from their name, even the opposite they hide a lot, exactly because of the simplicity of doing $delegate() at first sight you would assume it is just an anonymous function (which it might just be, but that is an argument I already explained in one of the other numerous issues you opened) for the delegate the only other fairly reasonable alternative will be handle which was changed awhile back(you can look at the history and see the issue which addresses the decision in detail), to prevent misuse of the delegate as an interface (by overwriting the same method) which I think is absolutely valid argument.

People tend to misuse tools most of the time and this PSR will become a problem if people start using the delegate as a middleware - there will be inconsistencies between implementations and usage - that will defeat the purpose of the PSRs in general. If one implementation used the delegate as middleware and the other did not, it cannot be replaced with another implementation since the semantics/behavior might change and that is the completely opposite of SOLID, namely LSP.

Also I agree with @jschreuder since I am really starting to think that you are mainly a troll, rather than presenting a valid argument.

schnittstabil commented 7 years ago

@DaGhostman You totally misunderstood my comment, didn't you? The issue is:

The MIDDLEWARE-META.md does not explain TO THE READER WHY process was chosen, and not handle, dispatch, foobar or whatever.


for the delegate the only other fairly reasonable alternative will be handle which was changed awhile back […] to prevent misuse of the delegate as an interface

No! While this is totally off-topic to the issue above, I exactly know why s/handle/process/ was done: https://github.com/php-fig/fig-standards/commit/833350d08b8c9d81a7cd01b0cfeb2374402ce28e:

Rename handle() to process()
Using the same method name for the stack and middleware is logical and
may reduce conflicts with existing middleware implementations.

While this decision was somewhat based on a reason, it is clearly not valid anymore – the stack interface is not part of PSR-15 anymore.

Btw, based on the same reasoning @shadowhand may could have changed StackInterface::process to StackInterface::handle as well to achieve the same result.


actually, make and resolve don't make sense in the context of a container.

While this is totally off-topic to the issue above too, please take a look at their statistical analysis: Auryn and Laravel use make; League\Di and Orno\Di use resolve.

schnittstabil commented 7 years ago

Now, since your major point is to use callable and __invoke

NO! It seems you totally misunderstood my intentions at #39 as well. From the very fist comment at #39:

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.

I've clearly explained that it was only a proof of concept. #39 was always about figuring out the pros and cons of the callable and interface idea, and we all agreed that callable comes with to many drawbacks, see my https://github.com/http-interop/http-middleware/pull/39#issuecomment-265276407:

@shadowhand Thanks for closing. I see that a PSR should not encourage callable middlewares.

schnittstabil commented 7 years ago

if people start using the delegate as a middleware

See #63, why this is still possible – the process name conflict cannot prevent that.

schnittstabil commented 7 years ago

If one implementation used the delegate as middleware and the other did not, it cannot be replaced with another implementation since the semantics/behavior might change and that is the completely opposite of SOLID, namely LSP.

Please explain that or at least give a PHP-example what you mean, and how this is related to the Liskov substitution principle:

lsp

Note: Maybe you want to use https://3v4l.org/ to make your point clear.

schnittstabil commented 7 years ago

@DaGhostman Btw, I'm able to mathematically prove:

I therefore look forward with a great deal of interest how LSP could be related with callable and/or __invoke interfaces. :heart: