Vanilla - added generic PSR-compatible middleware

[itsgoingd]

• Adds a generic PSR-compatible middleware to the vanilla integration.
• For use with applications and frameworks implementing PSR-15 (and by extension PSR-7 and PSR-17), eg. Slim, Mezzio, etc.
• Also improves the overall vanilla integration - using PSR request instead of globals if specified, allowing to set or auto-detect a PSR response factory.
• To allow the vanilla integration to generate responses a PSR-compatible response factory has to be provided, or the php-http/discovery has to be installed for zero-configuration use.
• This will replace the Slim-specific integration in a future major version.
• Based on an earlier PR by @UlrichEckhardt and a PR by @DominicDetta

Example use in a Slim application:

use Clockwork\Support\Vanilla\ClockworkMiddleware;
$app->add(ClockworkMiddleware::init()->withResponseFactory($app->getResponseFactory()));

// Run App & Emit Response
$response = $app->handle($request);

With the php-http/discovery package installed, the response factory will be auto-detected (if one is installed), making the initialization as simple as:

use Clockwork\Support\Vanilla\ClockworkMiddleware;
$app->add(ClockworkMiddleware::init());

You can also instantiate the middleware with a custom Clockwork instance (useful if you want to register the instance with a DI container for example) and disable routing if you want to set it up on your own:

use Clockwork\Support\Vanilla\Clockwork;
use Clockwork\Support\Vanilla\ClockworkMiddleware;
$clockwork = new Clockwork([ ... ]);
$app->add((new ClockworkMiddleware($clockwork))->withoutRouting());

$app->get('/__clockwork/{request:.+}', function ($request, $response) use ($clockwork) {
    return $clockwork->usePsrMessage($request, $response)->handleMetadata();
});

Note, because the Slim skeleton project registers the routing middleware as the last (outermost) middleware, you will need to add Clockwork middleware in public/index.php just before the $app->handle() call, instead of app/middleware.php.

[itsgoingd]

@UlrichEckhardt @DominicDetta Please check this PR out and let me know what you think about my take on this feature.

[UlrichEckhardt]

Just looked at the code a bit, so I haven't tried it yet, but that's now on my list for today. Here are a few remarks up front already.

[itsgoingd]

Thanks for the review, everything should be fixed now.

[itsgoingd]

An example Slim project is now also available here - https://github.com/underground-works/clockwork-examples/tree/master/Integrations/Slim#readme

[UlrichEckhardt]

Good morning!

I've been toying around with this a bit and found a few more issues:

• By above instructions, Clockwork isn't enabled. You need to pass ['enable' => true] to the Clockwork core being constructed or set an environment variable to achieve the same. With that, the browser plugin works now. This shouldn't be an issue if Changed default behavior  #686 is resolved by reverting my faulty change.
• The web frontend doesn't work for me. It copies some files around but then fails to serve them.
• I believe the path parts of the URLs for API and web frontend should come from the configuration. BTW, it's unclear to me still, what is the default for these? If I have my API at http://my.service/api/endpoint, the Clockwork API is then typically http://my.service/__clockwork/*, right? But where is the web frontend? Is it http://my.service/web or http://my.service/clockwork or something completely different?

That said, I personally find the implementation increasingly hard to follow. In particular the duality with the implementation switching between PSR and globals is making this difficult. Maybe making this a configuration flag which is set in the constructor would help a bit, splitting it into separate classes would be my preferred choice though, but that's a major change. One aspect of this is also that the core Clockwork class is volatile with respect to function parameters and return types. For example, Clockwork::response() either creates and returns a PSR response or it sends the response. So, depending on the use, it either returns something or causes a side effect, which is confusing for humans and also impossible to explain to a static analyzer (PHPStan, Psalm etc).

Just to prevent misunderstandings, I think this adds value and that it doesn't cause any regressions, so I'd be favorable to merging it, despite previous criticisms!

Uli

[UlrichEckhardt]

Just updated my demo repository:

• Branch feature/vanilla-psr-middleware -- this one's based on the code from this PR.
• Branch feature/psr-17-middleware -- This one's on my own implementation, just for comparison.

The reason I'm mentioning this is that both of these have PHPUnit tests included, which check whether the demo API, the Clockwork API and the Clockwork web UI works. The tests there differ also a bit because of my uncertainty about where the web UI is supposed to be located URL-wise.

Cheers! :)

[itsgoingd]

Thanks for the feedback! Yes, I assume in the examples, that the enable behavior is already changed. That will be done in a separate PR though.

Oops, the default setting for the web UI assets public path was one level off, so they were not actually copied to the public dir as they were supposed to. This should be resolved in the latest commit.

So let me try to explain how the paths work (and good thing you've mentioned this, because I've realised we are missing a bit of the implementation here):

• The default api path is /__clockwork. This is typically not configurable (eg. in the Laravel integration) as it is unlikely to clash with any existing routes and having a well-known default makes things easier for the client apps. We do allow to customize this in the Vanilla integration as an exception, mainly just to support the most primitive apps, where the app is literally just a bunch of .php files in a directory and you can't even route path like /__clockwork/whatever.
• The default web UI path is /clockwork. This is configurable in all integrations, as it is more likely to clash with other existing routes. Now in Laravel, there is a route catching all requests for the web UI assets and serving them manually. In the Vanilla integration, to keep things simple, the assets are copied directly to the public directory on first use, and the asset handling is left to the web server.
• Now when the app doesn't run in the root of the domain, but has a base path, let's say /app, the paths should be /app/__clockwork and /app/clockwork. This is obvious, as requests without the /app base might not be even routed to the app. An extra header also has to be set, to tell the browser extension what the base path is. In Laravel we auto-detect this base path, but I'm not sure how to do this in a generic way with the PSR-7 request. I guess we might need to require a manually configured path in that case.

I kind of agree the Vanilla helper class is starting to get a little bit complicated and might be a good candidate for a rework at some point. I'm currently assuming a 5.x release for this feature, so I didn't want to do any big changes at this time.

I especially don't like that part, where the response factory gets randomly passed as a third argument to the usePsrMessage() method. I might still move that part to the middleware and pass in an empty response ready to be used.

[UlrichEckhardt]

Heya!

Let's merge this, and then make it better. It definitely adds value already, so there is no point in holding it back.

Concerning the things to improve, the web UI isn't working out of the box for me yet. Key to this is actually documented: "Path where to install the Web UI assets, should be publicly accessible". For me, this is not the case since I route every request through my Slim stack. Yes, for e.g. Nginx, you can tell it to just serve the file if it exists and is not a *.php and that should be the default if you care for performance. However, it doesn't have to be the case and since I don't serve a website (with some static content) but an API, I'm not be prepared for serving of static files.

Let's get this moving!

Uli

[UlrichEckhardt]

That last changes looks fine, too. Let's merge this!