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

!!!TASK: Remove deprecated Http objects and replace with PSR-7 implementation #1552

Open
wants to merge 60 commits into
base: master
from

Conversation

4 participants
@albe
Copy link
Member

commented Apr 19, 2019

Related #658

TODOS:

  • Merge ActionResponse into HttpResponse and ComponentContext
  • Move merged arguments (postponed / won't do)
  • UploadedFiles -> untangledFiles array helper (won't do)
  • default SERVER params (in factory?!)
  • Move uploaded file to factories package
  • Merge sub request ActionResponses up
  • Get rid of ActionResponse inheritance
  • Fix tests
  • Get rid of generic Mvc\Request/ResponseInterface and separate Cli and Mvc Dispatchers
  • Streamline usage of PSR Factories
  • Revisit ActionRequest/ActionResponse API (postponed)
  • Get rid of HTTP stack
@kitsunet

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

OK, I totally missed this change and basically started the same in parallel. I will try to rebase my work on top of this, as I have a couple of changes that are not in here yet. We might want to discuss the Factories. I currently do not use them.

@albe

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

Thanks for the feedback! Yes, let's discuss things. Just hit me on slack sometime this week.

This is a pretty early status and I only got the first tests pass again as easily as possible, by jumping on the Request::create* stuff to avoid rewriting all those tests. This is not how it should be ofc. If you manage to rebase your work on this or merge them together, that would be awesome!

@kitsunet

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

There we go, tests will not run fully, but should be a step in the right direction. One thing to discuss is default SERVER vars as they know get applied with our request. That breaks a bunch of tests at the moment and I am not 100% sure how to fix this. Potentially could be fixed in the ServerRequestFactory but \o/ not sure

In general I didn't use the factories because in the request handler we are bound to a specific implementation anyways. It's nice to have for other components though.

@kitsunet kitsunet changed the title TASK: Remove deprecated Http objects and replace with PSR-7 implementation !!!TASK: Remove deprecated Http objects and replace with PSR-7 implementation Apr 29, 2019

@albe
Copy link
Member Author

left a comment

Some feedback

Show resolved Hide resolved Neos.Flow/Classes/Http/Component/BaseUriComponent.php
Show resolved Hide resolved Neos.Flow/Classes/Http/Client/Browser.php Outdated
Show resolved Hide resolved Neos.Flow/Classes/Http/Component/TrustedProxiesComponent.php Outdated
Show resolved Hide resolved Neos.Flow/Classes/Http/Request.php Outdated
Show resolved Hide resolved Neos.Flow/Classes/Mvc/DispatchComponent.php Outdated
Show resolved Hide resolved Neos.Flow/Classes/Mvc/Routing/RouterCachingService.php Outdated
Show resolved Hide resolved Neos.FluidAdaptor/Tests/Functional/View/StandaloneViewTest.php Outdated

kitsunet added some commits May 2, 2019

kitsunet added some commits May 2, 2019

@@ -32,6 +32,9 @@ class RequestBodyParsingComponent implements ComponentInterface
public function handle(ComponentContext $componentContext)
{
$httpRequest = $componentContext->getHttpRequest();
if (!empty($httpRequest->getParsedBody())) {

This comment has been minimized.

Copy link
@kitsunet

kitsunet May 5, 2019

Member

I have to check how we handled this before, but Guzzle ServerRequset::fromGlobals() sets $_POST as parsedBody, so if it was a "normal" POST request the parsed body will be fine already at this point.

This comment has been minimized.

Copy link
@albe

albe May 12, 2019

Author Member

Before we just merged $_POST into arguments on Request creation and kept the request body around until we parsed it when mapping arguments for the ActionController. Guzzle is more correct in assigning the POST as parsed Body, if it exists. It's just a PHP inbuilt request body parsing for multipart/form-data/application/x-www-form-urlencoded after all (which apparently only works for POST requests but e.g. not PUT).

@albe
Copy link
Member Author

left a comment

A bit of feedback. Generally, this isn't as weird as I thought, but it brings up a couple of locations, where our current logic is lacking, because we end up invoking ->render() and completely ignoring the return value.

kitsunet added some commits May 5, 2019

kitsunet and others added some commits May 9, 2019

@albe

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

🎉 green again, now we need to decide on the Uri thing.

@kitsunet

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Oh the CLI rename was quick. Given that it's not exactly the scope of this change I wanted to tackle that separately and ddiscuss a shim within the old namespace. Given that we never wanted to touch CLI in the first place I just wanted to make the necesary changes to keep it working. WDYT? @albe

@kitsunet

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I would rather not want to extend the implementation as that voids the point of the interface. Suddenly we rely on a specific implementation (again). Also the tests I fixed were functional tests were the URI read from a rendered content part was wrong, IMHO in that case we are more correct now because it should be encoded in the markup there. I would maybe have a look at how other implementations handle it and decide based on that.

@albe

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Given that we never wanted to touch CLI in the first place I just wanted to make the necesary changes to keep it working.

True, but since a Cli\Request and Cli\Response class never existed before, I think it makes sense to just introduce them with the name we want it to have finally. We can still put a shim in place, though I'm not entirely sure how that work and if, what that would improve.

We anyway need to think about a code migration plan.

I would maybe have a look at how other implementations handle it and decide based on that.

👍

@kitsunet

This comment has been minimized.

Copy link
Member

commented May 20, 2019

True, but since a Cli\Request and Cli\Response class never existed before

Huh? That's not true, they existed since ever?

@albe

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

They did?? Oh wait, yes, they just extended Mvc\Request/Response, no?

@kitsunet

This comment has been minimized.

Copy link
Member

commented May 20, 2019

The Request jusut implemented the Mvc\RequestInterface and the Response indeed extended the Mvc\Response class

@albe

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Fair enough. So yeah, would be cleaner to move that rename to an own PR. I'll take care this evening.

@@ -342,8 +342,8 @@ public function getControllerSubpackageKeyReturnsTheUnknownCasesPackageKeyIfNoCo
public function invalidControllerNames()
{
return [
[42],
[false],
//[42],

This comment has been minimized.

Copy link
@albe

albe May 21, 2019

Author Member

@kitsunet should we add an explicit test for the controller (and below action) name not being a "numeric" inside the setter to still pass these tests? I mean, a "42Controller" is an invalid class name anyway.

albe added some commits May 21, 2019

@albe
Copy link
Member Author

left a comment

Did a (half) review and left a couple comments for now.

@@ -46,9 +46,9 @@ public function handle(ComponentContext $componentContext)
{
$request = $componentContext->getHttpRequest();
$trustedRequest = $request->withAttribute(Request::ATTRIBUTE_TRUSTED_PROXY, $this->isFromTrustedProxy($request));
$trustedRequest = $request->withAttribute(ServerRequestAttributes::TRUSTED_PROXY, $this->isFromTrustedProxy($request));

This comment has been minimized.

Copy link
@albe

albe May 21, 2019

Author Member

We could provide a code migration for this.

Show resolved Hide resolved Neos.Flow/Classes/Mvc/ActionRequestFromHttpTrait.php Outdated
Show resolved Hide resolved Neos.Flow/Classes/Mvc/Routing/RoutingComponent.php Outdated
Show resolved Hide resolved Neos.Flow/Classes/Package.php
@@ -11,7 +11,7 @@
* source code.
*/
use Neos\Flow\Mvc\RequestInterface;

This comment has been minimized.

Copy link
@albe

albe May 21, 2019

Author Member

Are RequestPatterns used for CLI requests? I don't think so, but not 100% sure

This comment has been minimized.

Copy link
@kitsunet

kitsunet Jun 11, 2019

Member

Nope wouldn't make sense, the whole security stack depends on ActionRequests atm... If it does at some point we need to rethink that anyway.

@@ -132,7 +142,7 @@ public function argumentsOfPutRequestArePassedToAction()
*/
public function notFoundStatusIsReturnedIfASpecifiedObjectCantBeFound()
{
$request = Request::create(new Uri('http://localhost/test/mvc/actioncontrollertestc/non-existing-id'), 'GET');
$request = new ServerRequest('GET', new Uri('http://localhost/test/mvc/actioncontrollertestc/non-existing-id'));

This comment has been minimized.

Copy link
@albe

albe May 21, 2019

Author Member

why not serverRequestFactory?

@@ -26,7 +26,7 @@ class ActionRequestTest extends FunctionalTestCase
*/
public function actionRequestStripsParentHttpRequest()
{
$httpRequest = Request::create(new Uri('http://neos.io'));
$httpRequest = new ServerRequest('GET', new Uri('http://neos.io'));

This comment has been minimized.

Copy link
@albe

albe May 21, 2019

Author Member

ServerRequestFactory?

albe added some commits May 21, 2019

@albe albe added this to In progress in Neos 5.0 & Flow 6.0 Release Board via automation May 22, 2019

* @param ControllerInterface $controller
* @return void
*/
protected function emitBeforeControllerInvocation(Request $request, Response $response, ControllerInterface $controller)

This comment has been minimized.

Copy link
@kitsunet

kitsunet Jun 9, 2019

Member

We also need to do a final iteration on these signals and decide how we want to proceed in the future. I think it woudl be nice if there was two distinct signals, one for any controller invocation and one specific to CLI and MVC.

This comment has been minimized.

Copy link
@kitsunet

kitsunet Jun 11, 2019

Member

Ok, so the dispatchers irk me a bit. I kind of want an interface for dispatchers now, but the handle method arguments are different and not reconcilable in a way (the reason why we have two dispatchers now). I am thinking, the dispatcher is semi internal anyways, it's not api, so what if we declared this interface (not super helpful but at least something):

/**
 * @return mixed some response to the handled request, depending on the implementation
 */
public function handle();

And then instead of making the dispatcher implementations singleton, just have a constructor which gets the request, response (maybe even optional) and possible dependencies. The constructor would not be part of the interface then but the interface can point to the fact that you need to create implementations with some input (request).

So basically you could do something like

$response = (new Dispatcher($request, $response, $someDependency))->handle();

That would allow us to have a signal on this interface and request implementations to emit a signal for the interface additionally to any other signals it emits. I know that is a lousy reason for this rather meh interface, but I am not sure how to produce a signal for any dispatched "request", it would surely be useful to have though.

@kitsunet

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Mmm, something has changed for the worse again (apart from the tests), because eg. redirecting to the authentication endpoint for Neos doesn't seem to work and URL encoding for the query arguments is wrong. The initial ? is encoded. I am very sure I tested both successfully before. Any ideas what you mgiht have changed?

@kitsunet

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Found the encoding problem.... in \Neos\Flow\Mvc\Routing\Dto\UriConstraints::applyTo the CONSTRAINT_PATH does include a path AND the query string, which then (correctly) gets encoded on withPath... (ll 273).

Guess that needs refactoring.

@bwaidelich
Copy link
Member

left a comment

This makes a lot of sense to me!
I just added a nitpick comment regarding some superfluous cloning.

Apart from that, a case should be added to UriConstraintsTest (unfortunately I didn't include a test case that contains a path with query that would break now :-/)

And we have to make sure that RouterCachingService::storeResolvedUriConstraints() stores the query string in the cache, too. But IMO we don't need to adjust code for that – Except that we might want to call extractUuids() on the Query constraint now to properly tag cache entries that contain UUIDs in the query part

}
$this->resolvedUriConstraints = $this->resolvedUriConstraints->withQueryString($queryString);

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jun 12, 2019

Member

This line (and with it the cloning of the UriConstraints object) is only required if $queryString is not empty.
Therefore I'd suggest to keep the check here. Or (maybe better): Adjust UriConstraints::withQueryString() so that it returns the current instance if the given query string differs from the current value

This comment has been minimized.

Copy link
@kitsunet

kitsunet Jun 12, 2019

Member

Right, yes, I can leave that in, I figured it'S rather unnecessary because it defaults to an empty string in 525 so it would just be set to empty string then and teh overheat to do withQueryString seemed not too big. But the condition is no big deal. The bigger question is if you are ok with the addition of the extra constraint in general.

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jun 12, 2019

Member

My point was: Even if it's an empty string, the UriConstraints object will be cloned now.
But, sure, that's just a nitpick. The constraint in general makes a lot of sense to me (especially since path does not include the query per (PSR) definition)

* @return UriConstraints
*/
public function withQueryString(string $queryString): self
{

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jun 13, 2019

Member
Suggested change
{
{
$currentQueryString = $this->constraints[self::CONSTRAINT_QUERY_STRING] ?? '';
if ($queryString === $currentQueryString) {
return $this;
}

that's roughly what I had in mind btw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.