flow-development-collection icon indicating copy to clipboard operation
flow-development-collection copied to clipboard

Accidental? Feature removal: <form> tunnel request methods via field `__method`

Open mhsdesign opened this issue 3 years ago • 4 comments

This documentation is not up to date in projects since Flow 7.0: https://flowframework.readthedocs.io/en/stable/TheDefinitiveGuide/PartIII/Http.html?highlight=X-HTTP-Method-Override#request-methods

@kitsunet said

interesting, that seems to have been lost with the PSR-7 adoption, I found it back in Flow 5.3. We could/should re-add it

so to tunnel request methods like this wont work anymore: <input type="hidden" name="__method" value="PUT" />

(in the code base is not a single occurrence of __method)

mhsdesign avatar Jun 21 '22 08:06 mhsdesign

This was indeed accidentally removed with the switch to PSR-7. Last trace of this feature: https://github.com/neos/flow-development-collection/pull/1552/files#diff-be1687417ba866cbb06cf3eefe5cf80f9094a5890daa999b82b961f2a35df06aL115 And here the tests that should have covered it: https://github.com/neos/flow-development-collection/pull/1552/files#diff-ecafdf3d5592b4478c12c4195051c49f8b65cd1aeb1de5282495c9e6cbdbf890L176 So I'm not innocent on the disappearing of this

  • Unit Tests sometimes just don't suffice at all to make sure a feature doesn't perish, because if the original class vanishes the test for this class also does (or maybe that just shouldn't happen?!)
  • How do we want to make sure that every ServerRequest instance created like guzzles ServerRequest::fromGlobals() we currently use, does apply a logic to parse the request method from an override? Own factory that should only be used, effectively cutting back on the flexibility to swap the PSR implementation? Probably rather a new middleware, but then it only applies to requests that have passed through the middleware chain and not for ones that are created ad-hoc (we have this e.g. in the StandaloneView as a fallback)

albe avatar Jun 21 '22 09:06 albe

Unit Tests sometimes just don't suffice at all to make sure a feature doesn't perish

In theory they should, as in: nobody should just remove a test without thinking twice. This case was a bit special just because of the sheer amount of broken tests with the whole HTTP stack being replaced.

How do we want to make sure that every ServerRequest instance created like guzzles ServerRequest::fromGlobals() we currently use, does apply a logic to parse the request method from an override?

ServerRequest::fromGlobals() should only be used in cases where a completely fresh request is needed (e.g. for CLI). Otherwise we should get the "current server request" from the middleware (or via Bootstrap::getActiveRequestHandler() work around if not possible otherwise).

So, we could use s.th. like https://github.com/middlewares/method-override (or create a custom one / extend an existing one)

bwaidelich avatar Jun 21 '22 11:06 bwaidelich

Maybe we should just adjust the documentation on how to install/configure that middleware if this feature is required

bwaidelich avatar Jun 21 '22 11:06 bwaidelich

In case we do want to add an own middleware as a BUGFIX, I created a PR for that. We could also adjust the documentation and hint towards installing some arbitrary middleware.

albe avatar Jun 23 '22 12:06 albe