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

Slim 4 Beta Release Feedback #2697

Closed
l0gicgate opened this issue May 19, 2019 · 31 comments
Closed

Slim 4 Beta Release Feedback #2697

l0gicgate opened this issue May 19, 2019 · 31 comments
Labels

Comments

@l0gicgate
Copy link
Member

l0gicgate commented May 19, 2019

Slim 4 Beta Release
See the full release notes. You may also look at the Alpha release notes for more details about this major release.

Before doing anything read the docs
I just finished the first draft of the docs for Slim 4 which are available here. I need feedback please:
http://dev.slimframework.com/docs/v4

Download and test the 4.0.0-beta release
You may also play around with the 4.x branch and create a simple app with it to test things out.
composer require slim/slim:4.0.0-beta

Install a PSR-7 Implementation
You will also need to install a PSR-7 implementation and ServerRequestCreator combo. You can use Slim's PSR-7 Implementation or choose one of the ones described on the 4.x branch README: https://github.com/slimphp/Slim/blob/4.x/README.md
composer require slim/psr7

If you have any questions don't hesitate to ping me on Slack or ask questions in this thread directly, I'm available to help!

@l0gicgate l0gicgate pinned this issue May 19, 2019
This was referenced May 19, 2019
@Incisakura
Copy link

follow #2653 (comment)

<?php
use Slim\Http\Decorators\ServerRequestDecorator;
use Slim\Factory\ServerRequestCreatorFactory;
use DI\Container;
use Slim\Factory\AppFactory;
use Slim\Interfaces\RouteCollectorProxyInterface as RouteCollectorProxy;

$container = new Container();
AppFactory::setContainer($container);
$app = AppFactory::create();

$app->group('/user', function (RouteCollectorProxy $group) {
    $group->put('/ticket/{id}', UserController::class . ':ticket_update');
}

$serverRequestCreator = ServerRequestCreatorFactory::create();
$request = $serverRequestCreator->createServerRequestFromGlobals();
$request = new ServerRequestDecorator($request);
$response = $app->handle($request);

class UserController
{
    public function update($request, $response, $args)
    {
        $content = $request->getParam('content');
        var_dump($content);
        exit();
    }
}

My posted form data is

content: test

But I just got a NULL in response.
Could you tell my is there anything wrong in my code?

@RyanNerd
Copy link
Contributor

@SakuraSa233 The code $content = $request->getParam('content'); should be replaced with:
$content = $request->getParsedBody();

@l0gicgate
Copy link
Member Author

@SakuraSa233 first of all you don’t need to do any of this boilerplate code anymore. Update to 4.0.0-beta and the decoration is done automatically for both request/response objects as long as Slim-Http is installed.

Also please make sure you’re using the latest version of Slim-Http as there have been some recent changes.

From the code I see, the decoration should work though and getParam() should work but not for the purpose you’re intending to use. As @RyanNerd pointed out you need to use $request->getParsedBodyParam(‘content’) and it should return test.

@Incisakura
Copy link

Incisakura commented May 20, 2019

I tried it on slim/slim:4.x-dev 06682f1 and slim/http:dev-master 473a811. But all the result is just a NULL or array(0) {}.
In slim/slim:3.x, $request->getParsedBody() would return array(1) {["content"]=>string(4) "test"}, and getParsedBodyParam('content') would return test as expected.
I don't know where is wrong or I mistook something...

@l0gicgate
Copy link
Member Author

l0gicgate commented May 20, 2019

@SakuraSa233 in order for the body to be parsed it needs to be a POST request. In case you want the body to be parsed but use a PUT request you could use the packaged MethodOverrideMiddleware and set the X-Http-Method-Override header from the client side to the value PUT while making a POST request. That will ensure that the body gets parsed but that the PUT route gets invoked from the server side.

Your incoming request should look like this

POST /user/ticket/1 HTTP/1.1
Content-Type: application/x-www-form-urlencoded
X-Http-Method-Override: PUT

Add the method override middleware to your app

<?php
use DI\Container;
use Slim\Factory\AppFactory;
use Slim\Interfaces\RouteCollectorProxyInterface as RouteCollectorProxy;
use Slim\Middleware\MethodOverrideMiddleware;

$container = new Container();
AppFactory::setContainer($container);

// Create App and add MethodOverrideMiddleware
$app = AppFactory::create();
$app->add(MethodOverrideMiddleware::class);

// Add Routes
$app->group('/user', function (RouteCollectorProxy $group) {
    $group->put('/ticket/{id}', UserController::class . ':ticket_update');
}

$app->run();

@Incisakura
Copy link

Oh!!! That should be this...
Thanks for your patient reply!

@tuupola
Copy link
Contributor

tuupola commented May 20, 2019

I think the current behaviour is not correct. The X-Http-Method-Override is just a workaround for clients which do not support PUT, PATCH etc methods. Server should support them also when using the real request method.

Example Slim 3 code:

require __DIR__ . "/vendor/autoload.php";

$app = new \Slim\App;

$app->put("/test", function ($request, $response) {
    var_dump($request->getParsedBody());
});

$app->run();

yields:

$ curl "http://127.0.0.1:8080/test" \
    --request PUT \
    --include \
    --header "Content-Type: application/json" \
    --data '{ "foo": "bar", "pop": 66 }'

HTTP/1.1 200 OK
Host: 127.0.0.1:8080
Date: Mon, 20 May 2019 07:43:37 +0000
Connection: close
X-Powered-By: PHP/7.3.0
Content-Type: text/html; charset=UTF-8
Content-Length: 65

array(2) {
  ["foo"]=>
  string(3) "bar"
  ["pop"]=>
  int(66)
}
$ curl "http://127.0.0.1:8080/test" \
    --request POST \
    --include \
    --header "Content-Type: application/json" \
    --header "X-Http-Method-Override: PUT" \
    --data '{ "foo": "bar", "pop": 66 }'

HTTP/1.1 200 OK
Host: 127.0.0.1:8080
Date: Mon, 20 May 2019 07:44:25 +0000
Connection: close
X-Powered-By: PHP/7.3.0
Content-Type: text/html; charset=UTF-8
Content-Length: 65

array(2) {
  ["foo"]=>
  string(3) "bar"
  ["pop"]=>
  int(66)
}

Similar Slim 4 version:

use Slim\Factory\AppFactory;
use Slim\Middleware\RoutingMiddleware;
use Slim\Middleware\MethodOverrideMiddleware;

require __DIR__ . "/../vendor/autoload.php";

$app = AppFactory::create();

$routeResolver = $app->getRouteResolver();
$routingMiddleware = new RoutingMiddleware($routeResolver);
$app->add($routingMiddleware);

$methodOverrideMiddleware= new MethodOverrideMiddleware;
$app->add($methodOverrideMiddleware);

$app->put("/test", function($request, $response, $args) {
    var_dump($request->getParsedBody());
    return $response;
});


$app->run();

yields:

$ curl "http://127.0.0.1:8080/test" \
    --request PUT \
    --include \
    --header "Content-Type: application/json" \
    --data '{ "foo": "bar", "pop": 66 }'

HTTP/1.1 200 OK
Host: 127.0.0.1:8080
Date: Mon, 20 May 2019 07:49:26 +0000
Connection: close
X-Powered-By: PHP/7.3.0
Content-type: text/html; charset=UTF-8

NULL
$ curl "http://127.0.0.1:8080/test" \
    --request POST \
    --include \
    --header "Content-Type: application/json" \
    --header "X-Http-Method-Override: PUT" \
    --data '{ "foo": "bar", "pop": 66 }'
HTTP/1.1 200 OK
Host: 127.0.0.1:8080
Date: Mon, 20 May 2019 08:08:21 +0000
Connection: close
X-Powered-By: PHP/7.3.0
Content-type: text/html; charset=UTF-8

NULL

You can get some output by mimicking an old client which does not support PUT method natively.

$ curl "http://127.0.0.1:8080/test" \
    --request POST \
    --include \
    --header "Content-Type: application/x-www-form-urlencoded" \
    --header "X-Http-Method-Override: PUT" \
    --data '{ "foo": "bar", "pop": 66 }'
HTTP/1.1 200 OK
Host: 127.0.0.1:8080
Date: Mon, 20 May 2019 08:10:26 +0000
Connection: close
X-Powered-By: PHP/7.3.0
Content-type: text/html; charset=UTF-8

array(1) {
  ["{_"foo":_"bar",_"pop":_66_}"]=>
  string(0) ""
}

@l0gicgate
Copy link
Member Author

l0gicgate commented May 20, 2019

@tuupola there’s no media parsers in Slim-Psr7. It doesn’t handle JSON by default. As seen in your second last example you’re using application/json as Content-Type. It only supports form data out of the box so only application/x-www-form-urlencoded will work via a POST request. That’s because PHP doesn’t populate the $_POST object unless it’s a POST. If you install Slim-Http decorators then body parsing for JSON content works out of the box on POST requests.

Edit:
I just realized that Slim-Psr7 does not parse any type of bodies by default. Will be adding $_POST and application/json parsers shortly for next pre-release. All that functionality stayed in Slim-Http

@RyanNerd
Copy link
Contributor

@tuupola see this thread to implement an application/json media parser as middleware.

@tuupola
Copy link
Contributor

tuupola commented May 22, 2019

@l0gicgate Ah of course. Misunderstood the reason why $request->getParsedBody() returned null. Thanks!

@merlindiavova
Copy link
Contributor

Just a quick note
on https://www.slimframework.com/2019/04/25/slim-4.0.0-alpha-release.html#step-4-hello-world

// error
require __DIR__ . '/vendor/autoload.php.'; // there is a trailing dot after .php

// should be 
require __DIR__ . '/vendor/autoload.php'; // no trailing dot after .php

@adriansuter
Copy link
Contributor

@merlindiavova
Copy link
Contributor

@adriansuter made a pull request.

@scones
Copy link

scones commented May 31, 2019

I notice a lack of silent flag on App::run().

Additionally, there is an instantiation of a fixed class within the run method.

As i prefer testing the whole request, this prevents me from having more than one tests (headers already sent).
example from slim 3x:

$response = $app->run(true);

Should i just build a PR to re-enable this feature, or is this by design and there is another way to test in this fashion?

best regards,
scones

EDIT

answering my own question:

just use $response = $app->handle($request); :)

@http200it
Copy link

http200it commented Jun 1, 2019

There is some mistake. The HTTP 200 status is returned with the middleware of the example and the HTTP 404 return code

Sample code

$app->add(function (Request $request, RequestHandler $handler) {
$response = $handler->handle($request);
$existingContent = (string) $response->getBody();

$response = new Response();
$response->getBody()->write('BEFORE-' . $existingContent);
return $response;
});

$app->add(function (Request $request, RequestHandler $handler) {
$response = $handler->handle($request);
$response->getBody()->write('AFTER ');
return $response;
});

$app->get('/', function (Request $request, Response $response, $args) {

$response->getBody()->write('-Hello World  -');
//throw new Slim\Exception\HttpNotFoundException($request, "Product not found..."); // Does not work returns 200
return $response->withStatus(404); // <<<  Does not work returns 200
//return $response;
});

$app->run();

@bednic
Copy link
Contributor

bednic commented Jun 5, 2019

@http200it There is something bad with your snippet. I try it simulate on my local env and don't know what is your RequestHandler doing there,RequestHandleris invoke strategy for handling request to callbacks, but it is not Middleware, so handle method shoudl not exists. Slim 4 implements PSR MiddlewareInterface, so it's little bit different. If I just remove your middlewares before $app->get() and then try request I get 404 status, so everything seems alright. Maybe if you post you use namespaces I can simulate it better. But for now it looks like you use it bad way, looks like Slim 3 + Slim 4 mutantion, which cannot work properly.

@http200it
Copy link

http200it commented Jun 5, 2019

The 500 status return has been added, however this is ignored and is returned 200.

Step 4: Hello World >> http://slim-website.lgse.com/docs/v4/start/installation.html
+
Closure middleware example >> http://slim-website.lgse.com/docs/v4/concepts/middleware.html

Example code :
composer.json

{
    "require": {
        "slim/slim": "4.0.0-beta",
        "slim/psr7": "^0.3.0"
    }
}
<?php
// index.php
use Psr\Http\Message\ServerRequestInterface as Request;
use Psr\Http\Server\RequestHandlerInterface as RequestHandler;
use Slim\Factory\AppFactory;
use Slim\Psr7\Response;

require __DIR__ . '/vendor/autoload.php';

$app = AppFactory::create();

$app->add(function (Request $request, RequestHandler $handler) {
    $response = $handler->handle($request);
    $existingContent = (string) $response->getBody();

    $response = new Response();
    $response->getBody()->write('BEFORE ' . $existingContent);
    return $response;
});

$app->add(function (Request $request, RequestHandler $handler) {
    $response = $handler->handle($request);
    $response->getBody()->write(' AFTER');
    return $response;
});

$app->get('/', function (Request $request, Response $response, $args) {
    $response->getBody()->write("Hello world - Some application error!");
    return $response->withStatus(500);  //  Returns http 200 In this case, returning 500 simulates an error and deliberately returns HTTP 500 internal server error
});

$app->run();

In the $app->get('/' method, I intentionally return 500 and I think it should work.
I will add that if there is no middleware then it works.
I apologize for the language I am writing through the translator.

@bednic
Copy link
Contributor

bednic commented Jun 5, 2019

@http200it OK, now I get it where is the problem. Its your first registered middleware

$app->add(function (Request $request, RequestHandler $handler) {
    $response = $handler->handle($request);
    $existingContent = (string) $response->getBody();

    $response = new Response();
    $response->getBody()->write('BEFORE ' . $existingContent);
    return $response;
});

Because Slim 4 using LIFO (Last In First Out) so your first registered middleware (up there), will be executed last and there you creating new Response() therefore you get fresh response with default status 200:HTTP_OK and that is the reason why you get bad status code. You try commend your line:

$response = new Response();

It will work fine then.

@http200it
Copy link

I understand and thank you for the explanation.
Greetings :)

@Brammm
Copy link
Contributor

Brammm commented Jun 17, 2019

I just upgraded a small demo app from Slim 3 to this beta. Everything went fairly painlessly, following the upgrade guide. I was already using the PHP-DI bridge, which now seems no longer needed.

Biggest gotcha I ran into was this one: zendframework/zend-diactoros#278

I prefer extending App and loading my routes/setting middleware there instead of in an index.php file, so I couldn't use the AppFactory. I used zend diactoros as that was the one I knew, but apparently it doesn't do any content parsing out of the box... But reading the recommendations further etc, I'll probably try a different implementation seeing as diactoros is the slowest...

@l0gicgate
Copy link
Member Author

@Brammm why couldn't you use the AppFactory with Zend-Diactoros? It is supported out of the box with the PSR-7 auto-detection mechanism. As for content parsing you could simply use middleware to do so.

@Brammm
Copy link
Contributor

Brammm commented Jun 18, 2019

@l0gicgate This is my setup: https://github.com/Brammm/ProophSampleApp/blob/master/src/Api/Application.php

I did end up creating a simple middleware to do the content parsing.

@l0gicgate
Copy link
Member Author

@Brammm I see you're extending the App. I mean that's totally fine but then you can't leverage the automatic detection mechanism unfortunately. And I think that it might be a good idea for us to look into implementing some sort of content parsing middleware. It would probably be a great addition to the middleware collection we already ship.

@Brammm
Copy link
Contributor

Brammm commented Jun 18, 2019 via email

@scsmash3r
Copy link

scsmash3r commented Jun 26, 2019

@l0gicgate Using Slim v4. Reading docs: http://dev.slimframework.com/docs/v4/objects/request.html#route-object

Using Slim's PSR-7 implementation: https://github.com/slimphp/Slim-Psr7

Playing around with $request object.
Under the "Attributes" section it says:

The request object also has bulk functions as well. $request->getAttributes() and $request->withAttributes()

But in Slim's PSR-7 implementation in Request.php there is no such method available.

Slim v3 has withAttributes() method implemented: https://github.com/slimphp/Slim/blob/3.x/Slim/Http/Request.php#L956

Will this method be added back? Or it was intentionally removed for some reasons?

@adriansuter
Copy link
Contributor

@scsmash3r Slim PSR-7 follows strictly the PSR-7 definitions. You need to use a decorator like https://github.com/slimphp/Slim-Http to be able to access methods like withAttributes().

@glen-84
Copy link

glen-84 commented Jul 10, 2019

I'm wondering if it would be better to put the Twig view into the container using its FQCN instead of just view.

It could help with auto-wiring containers, for example:

use Slim\Views\Twig;

// Auto-wiring can work based on the type hint.
public function __construct(Twig $view) {}

Also, when using annotations currently:

/**
 * @Inject("view")
 * @var Twig
 */
protected $view;

Could be:

/**
 * @Inject
 * @var Twig
 */
protected $view;

Or when PHP 7.4 is released:

/** @Inject */
protected View $view;

Slim\Views\Twig is also more unique than just view, and can be referenced in a "constant" manner with ::class.

@adriansuter
Copy link
Contributor

There is an optional parameter called containerKey in the constructor of the twig middleware that can be used to achieve that. To avoid BC, this had been implemented like so.

@glen-84
Copy link

glen-84 commented Jul 10, 2019

@adriansuter Oh, nice. That change hasn't been released yet though. 🙂

@adriansuter
Copy link
Contributor

Not yet. And by the way, I totally agree. To add another reason to your list: my IDE understands the concept of containers and if I write $twig = $container->get(Twig::class); it detects the type for $twig automatically.

@l0gicgate
Copy link
Member Author

Closing this thread follow in #2770

@l0gicgate l0gicgate unpinned this issue Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests