2.0 Proposal

[mbabker]

See #343 for details

I'm leaving it as split-up commits to make it a little easier to see how everything evolved.

If accepted as is, what I'd personally do is this:

• Merge these PRs:
  • Fix deprecations on Symfony 6.1 #328
  • Fix php 7.4 pipelines #344
  • [1.x] Adjust CI setup to get functional MongoDB install again #348
  • [1.x] Add forward compat layer for changed logout listener configuration #350
• Cut a 1.4 release
• Merge this
• Cut a 2.0 beta

[maxhelias]

We should depreciate it before we delete it

[mbabker]

Yeah, thanks for the reminder here. There are definitely a few things that could use a proper deprecation in a 1.x release that I'm now forgetting about since I've been sitting on this branch for so long 😬

[mbabker]

#350 for compat layer

[maxhelias]

BTW, we should not overload a new response if one exists and let the "DefaultLogoutListener" set it
And also set a negative priority to be before the "CookieClearingLogoutListener"

[mbabker]

BTW, we should not overload a new response if one exists and let the "DefaultLogoutListener" set it

Well, this is a bit of a chicken vs. egg problem. The DefaultLogoutListener::onLogout() listener has a priority of 64, which makes it the first event listener to run on the logout event in the app for one of my company's bigger clients. So an if ($event->getResponse() === null) check isn't going to have a lot of value here without making it a higher priority than the default listener.

And also set a negative priority to be before the "CookieClearingLogoutListener"

The CookieClearingLogoutListener::onLogout() listener already has a priority of -255, so this listener is already running before that one (when it's enabled).

[maxhelias]

Tell me if I'm wrong: in case your application uses a form_login on the same firewall as the refresh_jwt, the {firewall}.logout.path configuration will not work anymore because the response created by the DefaultLogoutListener will be overwritten by this listener.
And BTW, even in the current configuration of this bundle, we should have this case

[mbabker]

In that scenario, yeah, you're hosed.

This almost needs a generate_response config param TBH, one can then explicitly opt into whether this bundle generates (and potentially overwrites a previously set) response for the logout request.

You then need to split the cookie invalidation part of the listener off into another lower priority listener (maybe the same priority as CookieClearingLogoutListener::onLogout()) that way it runs as late in the process as possible and has a better chance of clearing the cookie on the right response object (in case another bundle or a listener in the app itself is doing something with the response).

[maxhelias]

Yeah a config on the refresh_jwt like logout_path to check if the route match to generate the json response or not

[shakaran]

@thomaspicquet Could you rebase the conflicts to keep this big one issue updated? Thanks

[deluxetom]

@mbabker I've been using your version 2.0 on production for a couple months now and its been working well ;)
if that helps to get this merged :)

[shakaran]

Please @markitosgv @mbabker, could we merge this for get rid of a lot deprecation warnings annoying? The 2.0 release is taking a lot time to happen.

We need remove the deprecations and old code and release the 2.0 clean for Symfony 7.0 world. It is very annoying see the deprecations warnings from 1.0 release or master.

All the uses from

use Gesdinet\JWTRefreshTokenBundle\Generator\RefreshTokenGeneratorInterface;

Instead

use Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenManagerInterface;

Are spamming a lot project logs with deprecated.

Thanks

[mbabker]

We need remove the deprecations and old code and release the 2.0 clean for Symfony 7.0 world. It is very annoying see the deprecations warnings from 1.0 release or master.

What deprecations, specifically, are you hitting with this bundle's latest 1.x release that cannot be fixed? The bundle itself shouldn't be using its own deprecated code. If your issue is just the presence of @deprecated annotations, that alone isn't a problem (the Symfony debug loader won't randomly spit out log messages for deprecated methods when they aren't called).

All the uses from

use Gesdinet\JWTRefreshTokenBundle\Generator\RefreshTokenGeneratorInterface;

Instead

use Gesdinet\JWTRefreshTokenBundle\Model\RefreshTokenManagerInterface;

Are spamming a lot project logs with deprecated.

Is something in your project using the deprecated RefreshTokenManagerInterface::create() method? Outside of the test suite, nothing in the bundle is calling it.

[shakaran]

Is something in your project using the deprecated RefreshTokenManagerInterface::create() method? Outside of the test suite, nothing in the bundle is calling it.

it some way, when I load the bundle, always hit https://github.com/markitosgv/JWTRefreshTokenBundle/blob/master/Entity/AbstractRefreshToken.php#L16 since the autoloaders with PSR4 load all the claseses.

Since it is a interface, and it doesn't have a constructor, it is always loaded the trigger of deprecation and it should be only show when it is really in use.

This was already mentioned at api-platform/core#4790 (comment)

Regarding to ::create(), I am already using createForUserWithTtl():

$refreshToken = $this->refreshTokenGenerator->createForUserWithTtl(user: $kUser, ttl: $this->ttl);

[mbabker]

it some way, when I load the bundle, always hit https://github.com/markitosgv/JWTRefreshTokenBundle/blob/master/Entity/AbstractRefreshToken.php#L16 since the autoloaders with PSR4 load all the claseses.

It's no different than trigger_deprecation() calls that live outside classes in Symfony files; files like the deprecated ContainerAwareTrait or RequestMatcher class would cause the same type of excess deprecation logging if they were arbitrarily included as well.

That deprecation being triggered isn't the autoloader arbitrarily including all files, but rather processes that eagerly scan and load files. I know API Platform has stuff that can do that (exactly as pointed out by that issue), moving those calls doesn't necessarily solve anything here (and if anything it's a higher risk change to introduce a constructor to an abstract class where one didn't exist previously).

[shakaran]

sn't the autoloader arbitrarily including all files, but rather processes that eagerly scan and load files. I know API Platform has stuff that can do that (exactly as pointed out by that issue), moving those calls doesn't necessarily solve anything

So, since we cannot avoid it, if 2.0 get released, then we can remove all trigger_deprecation() calls finally