rfc-expressive-router-1.1.md

[RFC] zend-expressive-router (and related) changes for Expressive 1.1.

Proposed zend-expressive-router changes include:

• Adding a $path parameter to RouteResult::fromRouteMatch().

  What if instead we were to add a new static method, RouteResult::fromRoute(), and a new instance method, RouteResult::getRoute()? (as I have suggested in zendframework/zend-expressive#398)? This would allow consumers to then pull the path from the Route instead, and provide access to the path, name, allowed methods, options, etc. (e.g., $result->getRoute()->getPath())

  This could even be done in a new 1.3.0 minor release; users who depend on that new feature would need to update their project to pin to the new minor release or later; otherwise, everything continues working as normal.

• Removal of RouteResultObserverInterface and RouteResultSubjectInterface.

  These definitely require a major version bump.

• Changes to RouterInterface::generateUri() signature.

  This also definitely requires a major version bump.

What's interesting about these last two features, however, is that zend-expressive does not use them, which means that technically, it could pin to ^1.3 || ^2.0 safely. Additionally, because the router and template implementations are installed at the project level, users who update would not get the 2.0 version unless they also update the constraints for their given router and template implementations. In other words, there's no breakage.

This is true of the zend-expressive-helpers package as well; it's required by the project, which means that if the developer attempts to update their router and/or template implementation without also updating the zend-expressive-helpers library, Composer will either do nothing, or report the constraint conflict. If you run composer update, nothing happens. If you want the new versions of these, you either edit the composer.json to update constraints, or you do something like composer update "zendframework/zend-expressive-helpers:^3.0" "zendframework/zend-expressive-router:^2.0" "zendframework/zend-expressive-fastroute:^2.0" "zendframework/zend-expressive-platesrenderer:^2.0". This is something we can easily document in the migration guide.

As such, I think if we were to follow my recommendation about a new RouteResult::fromRoute() method, we could do the following:

• A 1.3.0 release of zend-expressive-router with the RouteResult change, as well as the other changes suggested in zendframework/zend-expressive#398.

• A 2.0.0 release of zend-expressive-router with the removal of the RouteResult*Interfaces, and the RouterInterface::generateUri() signature changes.

• New minor releases of each router implementation that pin to the zend-expressive-router 1.3.0 release (so we can make use of the stored Route in the RouteResult instance).

• New major releases of each router implementation that pin to the zend-expressive-router 2.0.0 release (so we can update the generateUri() signatures).

• New major release of zend-expressive-helpers pinning to zend-expressive-router's 2.0.0 release, so that we can update UrlHelper::__invoke() and make use of the new generateUri() signature.

• New major releases of zend-expressive-(twig|plates|zendview)renderer packages that pin to the new zend-expressive-helpers major release, so that they may update their own helpers.

Expressive 1.1 can then pin to zend-expressive-router ^1.3 || ^2.0, and pin each of the routers listed in the dev requirements to their latest v1 minor releases OR 2.0 series (e.g., "zendframework/zend-expressive-fastroute": "^1.3 || ^2.0"; as the functionality tested in zend-expressive is unrelated to the BC changes they introduce, we can do this). We would also update the skeleton to prefer the latest versions of each.

How does this sound?

I get comment notifications via https://giscus.co/, so feel free to comment here if desired. Once we reach consensus, we can start opening issues and updating the Expressive 1.1 project.

I already had giscus enabled, but I'm not sure if I get notifications about replies or mentions.

Adding a $path parameter to RouteResult::fromRouteMatch().

I read the suggestion earlier today and I like it. It makes more sense to include the entire matched routes than in stead of only parts of it.

Removal of RouteResultObserverInterface and RouteResultSubjectInterface.
These definitely require a major version bump.

Not sure about the major version bump. They are not being used anymore internally since version 1.0.0.rc6. Also all the RouteResultObservers were deprecated in that version and it explicitly states that they will be removed in version 1.1 (not sure if they actually are). Doesn't that mean we can just remove them in a minor release? On the other hand to avoid surprises, a major release for this might be better,

Changes to RouterInterface::generateUri() signature.
This also definitely requires a major version bump.

It's been a few weeks but didn't we settle for something that didn't cause a BC break? But again, a major release might be better anyway.

The proposed releases are looking good to me.

Some more open issues:
Bugfixes:
zendframework/zend-expressive-fastroute#19
zendframework/zend-expressive-aurarouter#11
zendframework/zend-expressive-template#10 + zendframework/zend-expressive-twigrenderer#22
zendframework/zend-expressive-twigrenderer#19

BC break:
zendframework/zend-expressive-twigrenderer#15

If the generateUri() changes are no longer BC breaks, I would be fine with having just a new minor release of zend-expressive-router. The observer/subject interfaces were never intended for v1 usage, and, in fact, not consumed by any part of Expressive at this point, so I am also in agreement that they could be removed in that same minor release.

Doing so would simplify the steps needed dramatically. Thoughts, @michaelmoussa?

I didn't get a Giscus notification for that initial mention, so feel free to ping me via e-mail or Twitter if I don't respond to any followups for too long.

• Adding a $path parameter to RouteResult::fromRouteMatch().

  What if instead we were to add a new static method, RouteResult::fromRoute(), and a new instance method, RouteResult::getRoute()

  This make sense to me. I think it'd be a good idea as well to have the new ::fromRoute(...) method set whatever additional properties are going to be unique to it, and then proxy to ::fromRouteMatch(...) to do the rest in order to minimize the copy-pasta.

  Should ::fromRouteMatch(...) be deprecated as well for removal in the next major release (whenever that may be), since it's going to end up being a subset of what ::fromRoute(...) does, or should we leave it alone?

• Removal of RouteResultObserverInterface and RouteResultSubjectInterface + Changes to RouterInterface::generateUri() signature.

  What's interesting about these last two features, however, is that zend-expressive does not use them, which means that technically, it could pin to ^1.3 || ^2.0 safely.

  This is true but technically, Zend\Expressive\Application does implement Router\RouteResultSubjectInterface and has the various attach/detach methods, so if someone is using them, it would be a BC break. I'd be 100% OK with it if we were emitting deprecation warnings for them already, but it looks like we've only got phpDoc annotations for them (which makes me only 99% OK with it). The annotations do say it will all go away in 1.1, so anybody who has looked at the source code or has a decent IDE knows this, but, again, it's technically a BC break. As such, I want to flag that explicitly before going ahead with that approach. Yeah, they shouldn't be using it, but we can't guarantee it.

  Being the uncompromising advocate of CI and automated testing that I am, however, I'm totally OK with taking the 1.1 approach if you both agree with it after reading the above. The most basic of unit tests would catch all that, after all. :)

A 1.3.0 release of zend-expressive-router with the RouteResult change, as well as the other changes suggested in zendframework/zend-expressive#398.

I am behind on issue participation but will take a look at that one when I've got a good chunk of time, since it looks long. If the changes don't break BC, then I'm 👍 once I make the RouteResult changes.

/^New (minor|major) releases of /

Those all make sense to me, and it's nice that we'll be able to make the RouteResult change available in a minor release.

How does this sound?

Sounds good to me! 👍

Now that we've got a bit more of a focused plan and the release quandary is more clear, it should be easier to make some progress. I'll work on the items in the original comment as time arises for me, and hold off on the additional open issues that @xtreamwayz cited so we don't double up on the work.

Hi, @michaelmoussa! Thanks for the replies! Sorry for my own lateness; busy few days.

• Removal of RouteResultObserverInterface and RouteResultSubjectInterface

  We can definitely remove the RouteResult*Interface items, and update Application to no longer be a Subject. As noted elsewhere, these were never intended to be part of the stable API, which is why they were deprecated before the stable release, and target v1.1 for removal. They are stil around only to allow our beta/RC testers to update their code to the stable API following the release. We can consider this code as not covered by semver.

• Adding the Route instance to the RouteResult via a new method, fromRoute(), along with a related instance method getRoute()

  Sounds like we're all in agreement on this approach! The open questions are:

  • who will be responsible for creating the patch to zend-expressive-router?
  • should we deprecate the fromRouteMatch() method when we create it? My vote is "yes".

• generateUri() signature changes

  I'm still confused about this one. I see #6 adds an optional $options parameter to the interface's method, but that's also against the dev-2.0.0 branch. @xtreamwayz indicates that a BC-safe approach was found... but I'm not sure if that was in the zend-expressive-router package, or in the zend-expressive-helpers package. Could one of you clarify for me so I can better understand its impact on minor/major release cycles?

Assuming that @xtreamwayz is correct in his assertion about generateUri(), then I think we should proceed with a 1.3 release of zend-expressive-router with these changes and those proposed in zend-expressive#398, and then:

• Update each of the router implementations:
  • to pin to zend-expressive-router 1.3+
  • to use the new fromRoute() method when populating a RouteResult
  • to ensure HEAD and OPTIONS requests are always honored
  • to issue new minor versions
• Update zend-expressive develop branch to pin to zend-expressive-router 1.3
• Update zend-expressive-skeleton develop branch to pin to the new minor versions of the router implementations

For my part, I'll:

• Create a patch for zend-expressive-router with the fromRoute() implementation.
• Work on the OPTIONS/HEAD changes for zend-expressive-router, and, once we have the minor release, the updates to each of the router implementations.
• Review the PRs that @xtreamwayz has linked.

@michaelmoussa and @xtreamwayz — zend-expressive-router has been updated, as have each of the implementations. All have new minor releases. (In the case of the Aura implementation, several patch releases as well, as I discovered subtle differences in how v3 of Aura.Router works in comparison to v2!) I've also got functionality added to zend-expressive, though it's not as proposed: it's new, optional functionality via 2 new middleware (ImplicitHeadMiddleware and ImplicitOptionsMiddleware), which make use of the new zend-router features.

From what I can see, then, the only things still in play are the changes to zend-expressive-helper. Those do not impact zend-expressive, but will impact the skeleton application. Michael, let me know if there's anything I can do to assist in that release.

At this point, my plan is to wait until after the holidays for the 1.1 release, as there's a number of other moving parts that need to become stable before we can finish out.

@weierophinney I responded to your e-mail from a few days back with some items to review for a couple of the releases.

I actually did not get any notifications for this Gist despite having Giscus authorized... I even checked my spam, so not sure what's going on...

I'm totally fine with continuing to collaborate via Gist for ease for formatting / posterity / etc., but if so, it'd probably be best to ping me via e-mail to check on a response until such time that the notifications work properly.