Suggestion for '<Dispatch|Route>Middleware::class' in 3.0alpha

[belgattitude]

Is there is a better way to resolve DispatchMiddleware and RouteMiddleware in the config/pipeline.php ?

See:

use Zend\Expressive\Middleware\DispatchMiddleware;  // unexistent class
use Zend\Expressive\Middleware\RouteMiddleware; // unexistent class
(...)
$app->pipe(RouteMiddleware::class); // = ->pipe('Zend\Expressive\Middleware\DispatchMiddleware');
$app->pipe(DispatchMiddleware::class); // = ->pipe('Zend\Expressive\Middleware\DispatchMiddleware');

IMO I would not rely on the ::class feature to avoid writing a string, but introduce some constants in expressive to know what we're dealing with.

Another option is to write the FQCN as a string (i.e $app->pipe('Zend\Expressive\Middleware\DispatchMiddleware')). Not very fond of this, but at least code checkers, phpstan... won't complain.

BTW: Bravo for the release !!!

[geerteltink]

It is explained here: zendframework/zend-expressive#548 (comment)

They are "Virtual" classes; they're FQCNs that do not resolve to actual classes/interfaces. It's a design decision that has been made. In the past, strings were used as you propose. Using FQCNs felt better as some of them are leftovers for backward compatibility and a smooth transition from expressive 2.

In PHP Stan you can ignore specific error messages.

[belgattitude]

I got the idea of 'virtual classes'... but still thinking, is there not a better solution ? I remember being confused by the "Zend\Expressive\Delegate\DefaultDelegate" or the "Zend\Expressive\WhoopsPageHandler" before.

At first sight, the idea to add a use statement and call ::class looked appealing to me (magically all looks so clean :)). Afterwards, I'm not so sure and while migrating a v2 -> v3alpha, I was stuck looking for those classes (took me a while to figure out they're just supposed to be strings - migration was done on a pet-project here).

I read @weierophinney comment on zendframework/zend-expressive#548 (comment), and I understand the reasons for aliasing (migrations...), but still think if the idea is to provide built-in aliases, let's define them as constants somewhere (promote IDE friendliness and phpdoc).

IMO, at least if it does not happen in expressive, I would recommend to not use 'virtual classes' in the skeleton. It's a starter, why would we have a set up exclusions for static analyzers ?

BTW: For phpstan that's what I'm doing already, see here but what about others (psalm, ea inspections...).

[geerteltink]

I remember being confused by the "Zend\Expressive\Delegate\DefaultDelegate" or the "Zend\Expressive\WhoopsPageHandler" before.

Exactly this. Either way leads to confusion. There should have been a comment above the classes explaining exactly this.

... let's define them as constants somewhere (promote IDE friendliness and phpdoc).

I like this idea. Add a few constants in the Application class with a comment to explain it. That should prevent the confusion for developers and static analyzers.

/cc @weierophinney thoughts?

[weierophinney]

I'm not against it, though I struggle with the following:

• What values to use for these constants. Would they look like class names? Or something else? I want them to be unique and easily disambiguated, because when configuration caching is in play, we need to be able to easily identify them when inspecting configuration.
• What happens if we ever introduce an actual class for what is currently a virtual class? This goes along with the above question, in my mind, because, ideally, we could then switch to ::class notation instead of the constant name, but the two would resolve to the same value.
• What happens when the class used to exist, but now doesn't? As an example, the RouteMiddleware and DispatchMiddleware exist in v2, but do not in v3. I'd prefer that these names continue to work, without requiring developers to update their configuration. This goes along with the previous two points.
• Should these be constants of the Application class, or a relevant factory? It seems like having them be constants of the factory that creates them would make more sense, as often they have little to do with the actual Application class.

So, my proposal is this:

• Zend\Expressive\Container\RouteMiddlewareFactory::ROUTE_MIDDLEWARE would resolve to Zend\Expressive\Middleware\RouteMiddleware.
• Zend\Expressive\Container\DispatchMiddlewareFactory::DISPATCH_MIDDLEWARE would resolve to Zend\Expressive\Middleware\DispatchMiddleware.
• Zend\Expressive\Container\NotFoundHandlerFactory::DEFAULT_DELEGATE would resolve to Zend\Expressive\Delegate\DefaultDelegate. (I will be renaming NotFoundMiddleware to NotFoundHandler in an upcoming patch, as the handler variant has more use cases than the middleware; hence the naming of the factory in this case.)

Thoughts, @belgattitude and @xtreamwayz?

[geerteltink]

That looks like a good solution to prevent more confusion amongst users and keep the IDEs happy at the same time.

[belgattitude]

looks good. just need a little space to think about it. I'll let you know my thoughts in a day.

[belgattitude]

@weierophinney , @xtreamwayz

A few notes for reflexion :

What happens when the class used to exist, but now doesn't? As an example, the RouteMiddleware and DispatchMiddleware exist in v2, but do not in v3. I'd prefer that these names continue to work, without requiring developers to update their configuration.

In V2, RouteMiddleware or DispatchMiddleware were somehow hidden (internal is probably a better word) and, in my experience, I used to rely on $app->pipeRoutingMiddleware() or $app->pipeDispatchMiddleware() following the skeleton practices. (BTW, they used constants (DISPATCH_MIDDLEWARE = 'EXPRESSIVE_DISPATCH_MIDDLEWARE' and ROUTING_MIDDLEWARE = 'EXPRESSIVE_ROUTING_MIDDLEWARE in Application)

V3 looks more natural to me. While migrating I've just replaced $app->pipeRoutingMiddleware() by $app->pipe(RouteMiddleware::class) (same for dispatch... noticing the usage of virtual class...).

All that to say that I'm agree with your concern: I'd prefer that these names continue to work... but looks to me those classes were 'internal only'... On the surface I was just using 'ROUTING_MIDDLEWARE'...

I'm probably shamefully wrong on this ;) just expressing my experience while migrating.

Should these be constants of the Application class, or a relevant factory? It seems like having them be constants of the factory that creates them would make more sense, as often they have little to do with the actual Application class.

In v2 they were somehow in 'Application' (DISPATCH_MIDDLEWARE, ROUTING_MIDDLEWARE). Yes it can make sense that they belongs to their containers.

My only concern...

I would not resolve the constants to FQCN... they aren't. Just "built-ins 'expressive'-specific keys" (sorry for my poor wording here).

because when configuration caching is in play, we need to be able to easily identify them when inspecting configuration

IMHO, let's make it clear they are built-ins constants, not classes.

So my proposal:

• Zend\Expressive\Container\RouteMiddlewareFactory::ROUTE_MIDDLEWARE would resolve to 'EXPRESSIVE_ROUTING_MIDDLEWARE', just like in v2....

But please, just remember I'm by no way expert on this matter. So feel free to make what you think is best.

BTW I love expressive and especially the clear distinction between middleware and handlers. I must admit that I didn't feel comfortable with middleware/action where both could be anything ... Thanks a million to make it ! :D

[belgattitude]

Zend\Expressive\Container\NotFoundHandlerFactory::DEFAULT_DELEGATE would resolve to Zend\Expressive\Delegate\DefaultDelegate. (I will be renaming NotFoundMiddleware to NotFoundHandler in an upcoming patch, as the handler variant has more use cases than the middleware; hence the naming of the factory in this case.)

👍

[weierophinney]

All that to say that I'm agree with your concern: I'd prefer that these names continue to work... but looks to me those classes were 'internal only'... On the surface I was just using 'ROUTING_MIDDLEWARE'...

I'm probably shamefully wrong on this ;) just expressing my experience while migrating.

This is a planned change for v2.2.

As we prepare new major revisions, once the API stabilizes, we plan a new minor release of the existing major version in which we perform deprecations and backports in order to provide a path for users to prepare their applications for the new major version.

One of the changes already staged is pushing the RouteMiddleware and DispatchMiddleware to a new minor release of zend-expressive-router, and having the variants within zend-expressive extend those. In the pipe*Middleware() methods, we would emit an E_USER_DEPRECATED indicating users should use pipe() with the relevant service name instead. This was where the suggestion that they resolve to actual class names came from.

I'm not sure I like the idea of constant values that resolve to strings like EXPRESSIVE_ROUTING_MIDDLEWARE. Yes, I created these, but they're as much "magic" as having something that looks like a class name; I much prefer having the namespace in place. This becomes more relevant when you consider some of the classes we're deprecating, such as the NotFoundDelegate and the DefaultDelegate virtual service that already use namespace + class notation; any constants we define for those will have to resolve to the existing names — unless we introduce new constant values within 2.2 first.

At this point, we need to make a choice, and the choice needs to be consistently applied. We either go with constant values that look like class names, or we go with the ALL_CAPS_UNDERSCORE_SEPARATED_WORDS format. In the case of FQCN, we risk confusion by users with regards to having service names that look like classes, but do not resolve to classes; in the second case, we end up with service names that may not provide semantics for what types are expected.

Considering we can alleviate the semantic meanings with well-crafted names, the second choice (string values that do not look like FQCN) is likely the best in terms of preventing user confusion, much as I dislike it. 😄

I'll work on that in the coming days.

[belgattitude]

much as I dislike it. 😄

Fully agree ;)

If it helps, just a little recap with built-ins I might use as a developer (excluding virtual classes like ApplicationPipeline...):

Constant (or better name)	Value (or any better name)
Container\RouteMiddlewareFactory::ROUTE_MIDDLEWARE	'EXPRESSIVE_ROUTE_MIDDLEWARE'
Container\DispatchMiddlewareFactory::DISPATCH_MIDDLEWARE	'EXPRESSIVE_DISPATCH_MIDDLEWARE

---

The NotFoundHandlerFactory :

Constant 'or better name)	Value (or any better name)
Container\NotFoundHandlerFactory::NOT_FOUND_HANDLER	'EXPRESSIVE_NOT_FOUND_HANDLER'

(Important to remember usage in v2 of Zend\Expressive\Delegate\DefaultDelegate' => Delegate\NotFoundDelegate::class, of course everything can be aliased)

---

And why not (for consistency) ?

Constant (or better name)	Value (or any better name)
Container\WhoopsPageHandlerFactory::WHOOPS_PAGE_HANDLER	'EXPRESSIVE_WHOOPS_PAGE_HANDLER'
Container\WhoopsFactory::WHOOPS	'EXPRESSIVE_WHOOPS'

---

I must admit I'm not really 100% happy with it... I'll think overnight and let you know.

Thanks @weierophinney !

[belgattitude]

@weierophinney

on my way back home I though about something that you may like (or dislike ;)

What about using real classes instead ? Imagine:

<?php declare(strict_types=1);
namespace Zend\Expressive\Resolver;

/**
 * Example of documentation:
 * RouteDispatcher class is used to resolve the route dispatcher
 * middleware in zend-expressive applications... .
 * (we can add here things like deprecation, usage, notes...)
 */
final class RouteDispatcher {};

• the namespace 'Zend\Expressive\Resolver' is given as an example (I'm very bad at naming, could be 'builtins', 'constants', 'virtual aliases'. 'utility'.. ;))

Pros (most of the things apply for constants too)

• IDE friendly (autocompletion, finding doc, possible refactoring...)
• Static analyzers won't complain (phpstan, pslam, scrutinizer, ea inspections...)
• Allow documentation (and things like @deprecated, but I'm not use how it can help, need to think of it)
• Future proof: in case the autoloading in PHP becomes more strict, different... (opcache...).
• No collisions with introduced classes.
• Don't need to find a good naming for constants.

Cons

• More classes (but empty anyway, I guess it does not change anything in terms of perf)
• Need to find a good name for the namespace !

Notes

• Could be an interface too, but can lead to questions like 'do we need to suffix with Interface...). I prefer a final class.
• Once we find the correct namespace naming, we can do the same for all other virtual classes (ApplicationPipeline...) if needed.
• For concerns about migrations v2->v3 and namings... I assume route and dispatch middlewares were strictly internal, not sure if this solution works for everything.

Would it be a good option ? @xtreamwayz @weierophinney

[weierophinney]

Honestly, I think that will be more confusing for developers diving in trying to understand the code:

• As empty, final classes, they do not represent the instance type of the service in any way.
• They could potentially lead to more autoloading calls (though referring to them only via ::class notation will not trigger autoloading generally).
• More code to maintain, including more potential for BC breaks (we cannot remove any of these without a major version change).

It's that first point that is really sticky; it's the same issue with any of the solutions proposed. "Virtual classes" that do not resolve to a real class: developers are confused about why. Empty final classes that have no relation to the final service loaded: developers confused about the relation between them.

I think constants are the least offensive option here. We can even make them namespaced constants, which means no new classes to import (i.e., they will not be defined on a factory which you might end up not using if you override the service definition), no unexplained or unexpected relations to other classes (e.g., defining them in something like Zend\Expressive\Application, when they refer to a middleware defined in another package), and the opportunity to document their purpose using annotations (as phpDocumentor and IDEs will introspect the constant documentation).

[belgattitude]

Then it looks perfect to me ! thanks a lot for taking your time on this.

[weierophinney]

The linked zendframework/zend-expressive-router#50 demonstrates this, and I've also provided a patch at zendframework/zend-expressive#551 that does similarly in the zend-expressive package. The first uses CAPS_CASE values, while the second uses values that look like class names; this allows existing references to continue to work, while new implementations will use the constants themselves.

[weierophinney]

I'll update the skeleton once the zend-expressive pull request is merged.

[geerteltink]

Fixed in #222

[belgattitude]

@xtreamwayz .

Fixed in #222

#222 looks okay for ROUTER_INTERFACE. Not yet for DISPATCH_INTERFACE... Please ignore if it's already on the plan. otherwise just a reminder, both should be. Thanks

[weierophinney]

@belgattitude Thanks for the reminder; I'll make sure that zend-expressive has a constant defined that aliases to the new zend-expressive-router variant. Good catch!

[weierophinney]

@belgattitude —

The constants both exist in the zend-expressive package as of its 3.0.0alpha6 release. What #222 did was refer to the ROUTE_MIDDLEWARE constant, as we were concerned about pulling the service by both the alias and the canonical name; if they resulted in different instances, we could have a potential bug in the application. With the DispatchMiddleware, it's not an issue, as the pipeline file is likely the only place you'll refer to that service.

But it turns out that in all containers we support, aliases resolve to their final service, and we then check to see if the service has already been created. This means that if you request a service by its alias name, you'll get the same instance as if you had retrieved the service to which the alias resolves.

As such, if your pipeline still refers to the legacy DispatchMiddleware class, under v3, you'll get the one defined in the zend-expressive-router package, due to an alias we provide in zend-expressive (see here).

As such, we actually didn't need to update the skeleton to refer to the ROUTE_MIDDLEWARE constant; we can refer to the canonical name (Zend\Expressive\Router\Middleware\PathBasedRoutingMiddleware). I'll be backing out that change later, and deprecating the constant again under zend-expressive, as we can wean folks off of the legacy naming over the v3 lifecycle.

[belgattitude]

@weierophinney Fantastic ! No issues after update to alpha7&8.