slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.94k stars 1.95k forks source link

Slim 4 Release Feedback #2770

Closed l0gicgate closed 3 years ago

l0gicgate commented 5 years ago

Slim 4 Release See the full release notes.

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: https://www.slimframework.com/docs/v4

Download the 4.0.0 release composer require slim/slim:^4.0

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

You can also use the Slim 4 DDD API Skeleton to create a new project composer create-project slim/slim-skeleton [my-app-name]

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!

mosfetish commented 5 years ago

V3 Docs have a "First Application" item. Be good to see that for v4. Seems pretty essential to me. I note the "DDD Skeleton" above but an app setup guide is what I'm after.

Hill-98 commented 5 years ago

I found that the middleware in Slim 4 can't call the dependency container using $this as before, now he will throw an error: Using $this when not in object context, can you tell me how to fix it?

mnapoli commented 5 years ago

Hey! Since this is a feedback thread, here is my feedback ^^

On the home page (here https://www.slimframework.com/docs/v4/) there is an example application that is different from the one showed in "Installation" (here https://www.slimframework.com/docs/v4/start/installation.html). The one on the home page worries me a bit: why is there a thing about adding a routing and error middleware? Does this not work by default? (I hope it does, TBH I'm just out of bed and haven't read the whole docs)

To be honest I don't really care about the PSR-7 implementation and having to choose one and install it is a bit of a pain IMO. I understand decoupling from anything that impacts the user (e.g. the container), but I don't understand why decouple from internal dependencies (PSR-7 and the factories, even routing).

In the meantime I discovered the https://github.com/slimphp/Slim-Skeleton repository, and also discovered it is using PHP-DI (cooool!). I have a few comments there on how it is used and how it could be improved, should I post this here or there?

l0gicgate commented 5 years ago

@Hill-98 showing example code would be great. It sounds like you're trying to call $this from a static context.

l0gicgate commented 5 years ago

@mnapoli

On the home page (here https://www.slimframework.com/docs/v4/) there is an example application that is different from the one showed in "Installation" (here https://www.slimframework.com/docs/v4/start/installation.html). The one on the home page worries me a bit: why is there a thing about adding a routing and error middleware? Does this not work by default? (I hope it does, TBH I'm just out of bed and haven't read the whole docs)

RoutingMiddleware is added by default when a user does not manually add it. This is so you can control when routing is being performed. In Slim 3 we had the setting determineRouteBeforeAppMiddleware which dictated when the routing was being done. Now you can precisely decide when it is being done by changing it's order in the middleware queue.

As for ErrorMiddleware, it needs to be added manually since you would want it to be last in the queue (Last In First Out) of middleware so it catches all of the errors from within the middleware pipeline. In Slim 3 we had a lot of feedback on how tightly coupled error handling was to the core of the App component and it generated a lot of issues, hence why it has been extracted from core and now users have more control over it.

To be honest I don't really care about the PSR-7 implementation and having to choose one and install it is a bit of a pain IMO. I understand decoupling from anything that impacts the user (e.g. the container), but I don't understand why decouple from internal dependencies (PSR-7 and the factories, even routing).

As explained in the release notes, modularity was the primary focus for this release so we can give back more control to the end user and let them make their own design decisions, giving more flexibility as to what core components you want to use and enable you to drop in your own easily if need be.

While you may not want to choose your own PSR-7/Container implementation, I think that the additional flexibility is more important. This is why we have the Slim-Skeleton project, you're also more than welcome to create your own skeleton so other users can benefit from your library choices over ours.

In the meantime I discovered the https://github.com/slimphp/Slim-Skeleton repository, and also discovered it is using PHP-DI (cooool!). I have a few comments there on how it is used and how it could be improved, should I post this here or there?

I would suggest creating an issue over there, since we want to keep things as topical as possible

ghost commented 5 years ago

Did you guys change literally everything?? It's impossible to upgrade from a reasonably sized Slim3 app to 4.

l0gicgate commented 5 years ago

@Rutmeister in order to get Slim in a good spot for the future, we had to break a lot of things unfortunately. Obviously it’s going to be a lot of work to upgrade on a large project, I’m not sure I would recommend doing so.

Hill-98 commented 5 years ago

@l0gicgate This is my code:

Slim 3: I use the container provided by Slim and he works very well.

$app = new App();
$container = $app->getContainer();
$container["cache"] = function () {
    $redis = new Redis();
    $redis->pconnect(REDIS_HOST, 6379, 3, REDIS_PREFIX);
    $redis->setOption(REDIS::OPT_PREFIX, REDIS_PREFIX);
    return $redis;
};
$app->add(function (Request $request, Response $response, $next) {
    $cache = $this->get("cache");
    // .....
});
//.....

Slim 4: I am using a container provided by PHP-DI and the following code does not work.

$container = new Container();
$container->set("cache", function () {
    $redis = new Redis();
    $redis->pconnect(REDIS_HOST, 6379, 3, REDIS_PREFIX);
    $redis->setOption(REDIS::OPT_PREFIX, REDIS_PREFIX);
    return $redis;
});
AppFactory::setContainer($container);
$app = AppFactory::create();
$app->add(function (Request $request, RequestHandler $handler) {
    $cache = $this->get("cache");
    // .....
});
//.....
jacekkarczmarczyk commented 5 years ago

@l0gicgate some frameworks/libs provide an upgrade guide, list of steps you need to do to make your app compatible with new version, something like

Before:

App::useRouter($router)

After

$app = new App();
$app->addRouter($router)

Maybe there could be something like this for Slim too? See for example https://vuejs.org/v2/guide/migration.html or https://github.com/vuetifyjs/vuetify/releases/tag/v2.0.0#user-content-upgrade-guide

ghost commented 5 years ago

@l0gicgate Imma be real with you chief. This ain't it. Mainly because there's no DI-Bridge in my case. Do you know if that's going to be supported for v4 anytime soon? Really can't go without it.

Other thing that might be a bug. When you search using the search bar in the v4 documentation it often links to the v3 docs.

l0gicgate commented 5 years ago

@Rutmeister you don’t need DI bridge. You can use any PSR-11 container implementation you want with Slim 4.

Go look at https://github.com/slimphp/Slim-Skeleton it uses PHP-DI with Slim 4.

Also, the doc search is not broken, the new docs were just released so we had to trigger a recrawl via Agnolia which takes several days to complete. It’ll be fixed shortly.

l0gicgate commented 5 years ago

@jacekkarczmarczyk that’s a good idea. We should do that.

l0gicgate commented 5 years ago

@Hill-98 this is an issue with the MiddlewareDispatcher since it does not use CallableResolver the ContainerInterface never gets bound to the wrapping class: https://github.com/slimphp/Slim/blob/4.x/Slim/MiddlewareDispatcher.php#L196

A quick solution in the interim:

$app->add((function ($request, $handler) {...})->bindTo($container));
Hill-98 commented 5 years ago

@l0gicgate Thank you very much, it's great.

ghost commented 5 years ago

@l0gicgate sorry I think I asked my question wrong. The DI container is working great but I'm having trouble getting the different controller parameters to work like the di-bridge offered in v3. https://github.com/PHP-DI/Slim-Bridge#controller-parameters

From what I've seen it's not possible yet but perhaps I'm wrong.

l0gicgate commented 5 years ago

@Rutmeister porting DI-Bridge to Slim 4 shouldn’t actually too big of an undertaking. I might make a PR on that repo this week if I have time.

BusterNeece commented 5 years ago

@l0gicgate My goodness, this upgrade is amounting to more work than I expected! I appreciate the changes under the hood that made Slim PSR-agnostic, and I can see how these were overall necessary, but this is requiring a refactor of huge swaths of my codebase.

One big piece of feedback I have is that the name argument provided for Middleware added via $route->add() no longer routes through the CallableResolver at all. Thus, out of the box you can't build in custom resolver functionality like supporting additional function arguments or other things that end up being quite necessary with many middlewares.

I'm looking into how to override the entire MiddlewareDispatcher to allow for this, but I gotta say it's a real pain to even need to.

Update: it turns out you can't just provide a replacement MiddlewareDispatcher out of the box. It's created by Slim\App's constructor and has no getters/setters. You'd have to overwrite the actual Slim\App class itself just to add this relatively minor but very important functionality.

Is there any way we can just get this upstream?

mnapoli commented 5 years ago

Just stumbled upon this:

Could not detect any PSR-17 ResponseFactory implementations. Please install a supported implementation in order to use AppFactory::create(). See https://github.com/slimphp/Slim/blob/4.x/README.md for a list of supported implementations.

I understand why, but this doesn't make any sense to me as a user. The framework doesn't work out of the box. Furthermore, when I opened https://github.com/slimphp/Slim/blob/4.x/README.md I saw way too many choices. Which one should I choose? I mean, having contributed to PSR-15 and PSR-17 I'm still confused here.

Maybe it would be worth pre-installing one implementation so that the core of the framework works?

I understand and respect the choice about the DI container, because Slim can work without a DI container. But I think HTTP is different because the framework can't work without it then.

(side note: please consider this as feedback and not blind criticism of the release, I do appreciate how huge these things are, I just want to contribute to improve it)

l0gicgate commented 5 years ago

@mnapoli when installing via composer, it suggests installing Slim-Psr7.

We could maybe add a note to the README saying: “If unsure, just install Slim-Psr7”

l0gicgate commented 5 years ago

@SlvrEagle23 yea that is one drawback of the new PSR-15 dispatcher. We had to make some tough decisions in that regard.

I’m open to providing the ability to pass in your own MiddlewareDispatcher via the App constructor so you don’t have to extend the app to override it.

l0gicgate commented 5 years ago

@Hill-98 & @SlvrEagle23 see #2779

esetnik commented 5 years ago

After upgrading to v4 something broke related to request uri matching when using nginx as the web server. I am using the configuration from http://www.slimframework.com/docs/v4/start/web-servers.html with a slight modification since my application is mounted in a subdirectory (/api).

    location /api {
        try_files $uri /api/index.php$is_args$args;
    }

    # pass the PHP scripts to FastCGI server listening on socket
    location ~ \.php$ {
        try_files $uri =404;
        fastcgi_split_path_info ^(.+\.php)(/.+)$;
        fastcgi_pass php:9000;
        include fastcgi_params;
        fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name;
        fastcgi_param SCRIPT_NAME $fastcgi_script_name;
        fastcgi_param SERVER_NAME $host;
        fastcgi_buffers 16 16k; 
        fastcgi_buffer_size 32k;
        fastcgi_param X_REQUEST_ID $request_id;
        fastcgi_hide_header X-Powered-By;

It appears that basePath processing has been removed from Slim v4 and is not available in Slim-Psr7 even though it is documented as being available and functional.

Base Path
If your Slim application's front-controller lives in a physical subdirectory beneath your document root directory, you can fetch the HTTP request's physical base path (relative to the document root) with the Uri object's getBasePath() method. This will be an empty string if the Slim application is installed in the document root's top-most directory.

See https://github.com/slimphp/Slim-Psr7/issues/118

mosfetish commented 5 years ago

The framework doesn't work out of the box.

Might be time to go "frameworkless". Slim 4 now appears to be a slightly-opinionated assemblage of components, a template more than a framework, because as you say, a framework is meant to work out of the box.

It leaves new users (and even current users) scratching their heads as to which way to jump. At the very least, the docs should contain tutorials that go deeper than mere hints at possible implementations.

l0gicgate commented 5 years ago

@esetnik for the record Slim-Psr7 is still in pre-release stage so yes, it still needs some work. You’re more than welcome to raise a PR or use a different PSR-7 implementation.

l0gicgate commented 5 years ago

@mosfetish comments like yours make me cringe, you’re not contributing anything.

It’s funny that people like you come out of the woodworks after we work for two years on this project for free, all the discussions are in the open on the repo, all the decisions were made publicly and everyone is welcome to contribute and follow along as the project progresses.

Please refrain from making such stupid comments, they’re not welcomed here.

esetnik commented 5 years ago

@esetn for the record Slim-Psr7 is still in pre-release stage so yes, it still needs some work. You’re more than welcome to raise a PR or use a different PSR-7 implementation.

Do the authors have a recommended implementation that preserves the majority of functionality when migrating from v3 to v4? As this is the release feedback thread I feel it’s important to mention this as I tried to migrate today, but got stuck because my project utilizes multiple slim apps behind one nginx. Since this is found in the v4 docs but doesn’t currently work it should probably be removed from the docs until a viable alternative is available.

Otherwise great work on the upgrade. It’s a huge change but I think it will pay off in the long run.

l0gicgate commented 5 years ago

@esetnik I personally like Nyholm/psr7 and thank you for the feedback, it is appreciated. I know there’s still a lot of work to be done docs/wise and to the supporting repos, we’ll get there.

mosfetish commented 5 years ago

comments like yours make me cringe, you’re not contributing anything.

Instead of answering my concerns you choose to attack me personally. That's the very definition of cringe-worthy.

But then to another you answer:

I know there’s still a lot of work to be done docs/wise and to the supporting repos,

Which was exactly my point but in your brittleness you've chosen to attack instead of being constructive.

l0gicgate commented 5 years ago

Cool story @mosfetish I don’t care.

Might be time to go "frameworkless". Slim 4 now appears to be a slightly-opinionated assemblage of components, a template more than a framework, because as you say, a framework is meant to work out of the box.

This is the cringe worthy stupidity I’m referring to. At least @esetnik brought up a valid point.

mosfetish commented 5 years ago

I don’t care.

And that's your problem. Fortunately most people in the Slim community are helpful and don't take umbrage or have such low trigger threshold. Best you stay behind the scenes.

l0gicgate commented 5 years ago

or have such low trigger threshold

I don’t have time for nonsense.

Fortunately most people in the Slim community are helpful

Says the person who hasn’t contributed shit to this project. The irony.

Best you stay behind the scenes.

Best you stay away from this community if you don’t have any constructive input. Go develop “frameworkless” projects.

theodorejb commented 5 years ago

Can we please all be nice to each other and keep this thread on topic? It might be best to just delete the last 5 comments of the conversation.

l0gicgate commented 5 years ago

@theodorejb sometimes it's important to set some parameters. As I said, I don't have time for nonsense, I don't get paid to do this and it's a full time job. I want the discussion to be as pertinent as possible, perhaps other users who would maybe act in a similar fashion will be discouraged of doing so by reading the last 5 comments.

mnapoli commented 5 years ago

@l0gicgate calling their comment "stupid" is not that great. Let's try to keep this friendly. It is, of course, easy to understand that it's hard to read this when you worked years on the project, I understand how you may be really frustrated.

Awilum commented 5 years ago

is there any release estimates for upcoming future ?

l0gicgate commented 5 years ago

@awilum I would like to do smaller releases as often as possible. This is all incumbent on how much development happens as well.

I think that’s what you’re asking?

raoulduke commented 5 years ago

When I try to install using Slim Skeleton it gives me Slim 3. Is there a way I can specify that I want Slim 4?

composer create-project slim/slim-skeleton test

Installing slim/slim-skeleton (3.1.7)
  - Installing slim/slim-skeleton (3.1.7): Downloading (100%)
Created project in test
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 38 installs, 0 updates, 0 removals
  ...
  **- Installing slim/slim (3.12.1): Downloading (100%)**

I'm running this in a Homestead vm on a Windows machine.

mosfetish commented 5 years ago

When I try to install using Slim Skeleton it gives me Slim 3. Is there a way I can specify that I want Slim 4?

The packagist repository linked to as v4 skeleton is composing with Slim 3.1.x and PHP 5.6

There are other v4 skeletons if that helps, such as:

https://github.com/adriansuter/Slim4-Skeleton

raoulduke commented 5 years ago

@mosfetish Thanks for the tip, I didn't think of the packagist version. dev-master is the version that uses Slim 4 and PHP 7. Specifying the version at the end of the composer command lets me install the latest.

composer create-project slim/slim-skeleton [my-app-name] dev-master

https://packagist.org/packages/slim/slim-skeleton#dev-master

l0gicgate commented 5 years ago

@raoulduke I just tagged the master branch to 4.0.0 just now. It should default to the correct version now. Thanks for pointing that out.

bigtunacan commented 5 years ago

This ties back to some other issues and was mentioned here and on a couple of other tickets by @esetnik , but I have also ran into this issue with Slim 4 not dealing with routing correctly when projects reside in sub-directories. This worked just fine in both Slim 2 and Slim 3. In my case this is sitting behind Apache using virtual directories which is a fairly common scenario.

I was able to work around this by using a slight variation on the URI middleware that @esetnik shared on another ticket, but it does seem like that should be handled directly by Slim's routing middleware. Also of note, was that for Apache we were only able to get this working with Guzzle; it would not work with the Slim PSR7 at all, for whatever reason.

This is not a complaint, but rather a real question; what are the plans for supporting Slim 3 looking like? It is a hard upgrade for big apps from 3 to 4, but Slim 3 is rock solid and I have concerns starting new applications on Slim 4 today. Not sure when is the right time to make the switch for new applications.

l0gicgate commented 5 years ago

@bigtunacan as mentioned above, Slim-Psr7 is still in pre-release state and we need to correct this issue. This is not a Slim 4 issue, base path detection needs to be handled via the underlying PSR-7 implementation. RoutingMiddleware depends on the ServerRequestInterface that it's being fed and the underlying Uri object is the real issue here as it doesn't get populated properly currently if you're using Slim-Psr7

As for Slim 3, it is in maintenance mode only. No more features will be released.

bigtunacan commented 5 years ago

@l0gicgate I do understand that Slim-Psr7 is pre-release which is why I switched to Guzzle.

Unfortunately, that does not address the issue of sub-directory routing which is important and a quite common issue. All Psr7 implementations require some type of workaround for this which is very non-intuitive. I would think that this should be handled in the Slim RoutingMiddleware prior to the handoff to ServerRequestInterface, otherwise if this is fixed in Slim-Psr7 then it would seem like there is a hard dependency on Slim-Psr7 which seems the opposite of what you are trying to achieve?

l0gicgate commented 5 years ago

@bigtunacan how do you propose we implement that logic? I'm still waiting for someone to come up with a one solution fits all:

See this longstanding issue: https://github.com/slimphp/Slim/issues/2512

bigtunacan commented 5 years ago

@l0gicgate I don't claim to be an expert at PHP or Slim so please go easy on me, but I have submitted a pull request with an attempt at this. It is based on ideas from @esetnik

JorisDebonnet commented 5 years ago

A few small things in the docs:

[edit: see https://github.com/slimphp/Slim-Website/issues/415]

... oh and public/index.php of slim/skeleton has line 17 indented with a tab, not spaces.

l0gicgate commented 5 years ago

@JorisDebonnet thank you for pointing these issues out. Could you please open an issue on the Slim-Website repository? https://github.com/slimphp/Slim-Website

opolanco23 commented 5 years ago

Not sure if this is the right place to post it. I reviewed the v4 documentation and found this in the upgrade guide Slim’s App settings used to be a part of the container and they have now been decoupled from it. I see that in previous documentation you could pass an array of settings to the app as it was being constructed. Now is that not possible? I am new to using slim so any advice would be appreciated

l0gicgate commented 5 years ago

@opolanco23 you should open an issue on https://github.com/slimphp/Slim-Website

We need to remove that as settings have been entirely removed.

Ewertonv90 commented 5 years ago

[Language PT-Brazil]fiz um ambiente de testes, o Slim 4.0 está quebrando o código referente à minha API do Slim 3.12, porém, se for para abrir novos caminhos para o Slim eu já estou a procura de um Curso para reescrever minha API para a versão 4.0