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 Release Feedback #2770

Open
l0gicgate opened this issue Aug 1, 2019 · 47 comments

Comments

@l0gicgate
Copy link
Contributor

commented Aug 1, 2019

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 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

This comment has been minimized.

Copy link

commented Aug 2, 2019

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

This comment has been minimized.

Copy link

commented Aug 2, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

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

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@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

@Rutmeister

This comment has been minimized.

Copy link

commented Aug 2, 2019

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

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@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

This comment has been minimized.

Copy link

commented Aug 3, 2019

@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

This comment has been minimized.

Copy link

commented Aug 3, 2019

@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

@Rutmeister

This comment has been minimized.

Copy link

commented Aug 3, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

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

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

@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

This comment has been minimized.

Copy link

commented Aug 4, 2019

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

@Rutmeister

This comment has been minimized.

Copy link

commented Aug 4, 2019

@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.

@mnapoli mnapoli referenced this issue Aug 4, 2019
@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@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.

@SlvrEagle23

This comment has been minimized.

Copy link

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@esetnik

This comment has been minimized.

Copy link

commented Aug 5, 2019

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 slimphp/Slim-Psr7#118

@mosfetish

This comment has been minimized.

Copy link

commented Aug 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@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

This comment has been minimized.

Copy link

commented Aug 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

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

This comment has been minimized.

Copy link

commented Aug 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@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

This comment has been minimized.

Copy link

commented Aug 7, 2019

is there any release estimates for upcoming future ?

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link

commented Aug 7, 2019

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

This comment has been minimized.

Copy link

commented Aug 7, 2019

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

This comment has been minimized.

Copy link

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@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

This comment has been minimized.

Copy link

commented Aug 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@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

This comment has been minimized.

Copy link

commented Aug 16, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@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:
#2512

@bigtunacan

This comment has been minimized.

Copy link

commented Aug 16, 2019

@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

This comment has been minimized.

Copy link

commented Aug 16, 2019

A few small things in the docs:

  • /concepts/middleware.html has a broken link to Middleware-for-Slim-Framework-v4.x
  • /objects/routing.html under Any Route : which need*s* a __invoke() (maybe even an instead of a)
  • /middleware/error-handling.html: You can now map custom handlers. The 'now' implies an upgrade, but this should just document v4, no? Further down: The rendering is finally decoupled from the handling. Everything still works the way it previously did. -- same remark.
  • /middleware/output-buffering.html: PREPEND mode will create a new response body and append it to the existing response. Shouldn't that be "... and append the existing reponse to it"?

[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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@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

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