add disable access token saving feature #32
Conversation
src/LeagueOAuth2ServerBundle.php
Outdated
| @@ -44,17 +44,36 @@ private function configureSecurityExtension(ContainerBuilder $container): void | |||
| private function configureDoctrineExtension(ContainerBuilder $container): void | |||
| { | |||
| /** @var string $modelDirectory */ | |||
| $modelDirectory = realpath(__DIR__ . '/Resources/config/doctrine/model'); | |||
| $modelCommonDirectory = realpath(__DIR__ . '/Resources/config/doctrine/common'); | |||
There was a problem hiding this comment.
I'm not really comfortable with creating several XML mapping config files.
That's why in another PR I used the "php" approach. Have a look at https://github.com/thephpleague/oauth2-server-bundle/pull/25/files#diff-bbea8ca4dd205bd3df8e9ce6f139afa2852196075bea90a24f320ea3e449ba88R70.
Therefore it could be great if #25 could be merged before that PR (so you can base your mapping configuration on it)
There was a problem hiding this comment.
Ah yes good point, I will check to do the same
There was a problem hiding this comment.
For this point I will do it when #25 will be merged to avoid multiple conflicts
| @@ -256,6 +271,7 @@ private function configureDoctrinePersistence(ContainerBuilder $container, array | |||
| $container | |||
| ->findDefinition(AccessTokenManager::class) | |||
| ->replaceArgument(0, $entityManager) | |||
| ->replaceArgument('$disableAccessTokenSaving', $disableAccessTokenSaving) | |||
There was a problem hiding this comment.
We try to use the argument index instead. In that way, renaming the argument won't require any changes elsewhere.
| if ($disableAccessTokenSaving) { | ||
| $container->setParameter('league.oauth2_server.persistence.doctrine.access_token.disabled', true); | ||
| } else { | ||
| $container->setParameter('league.oauth2_server.persistence.doctrine.access_token.enabled', true); | ||
| } |
There was a problem hiding this comment.
And if you think it's needed, I'd rather did like that:
$container->setParameter('league.oauth2_server.persistence.doctrine.persist_access_token', !$disableAccessTokenSaving);There was a problem hiding this comment.
yes it's mandatory for configureDoctrineExtension. I need both because I didn't find how to invert boolean value from string parameter. But maybe with php approach I could remove it
| { | ||
| $container | ||
| ->findDefinition(InMemoryAccessTokenManager::class) | ||
| ->replaceArgument('$disableAccessTokenSaving', $disableAccessTokenSaving) |
There was a problem hiding this comment.
Same here
We try to use the argument index instead. In that way, renaming the argument won't require any changes elsewhere.
| $loader->load('access_token/default.php'); | ||
| } | ||
|
|
||
| $container->setParameter('league.oauth2_server.authorization_server.disable_access_token_saving', $config['disable_access_token_saving']); |
There was a problem hiding this comment.
Is that parameter needed? Moreover it's kind of redundant with league.oauth2_server.persistence.doctrine.access_token.disabled parameter
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function getNewToken(ClientEntityInterface $clientEntity, array $scopes, $userIdentifier = null) |
There was a problem hiding this comment.
Yes I can it was not on the AccessTokenRepository so I did the same 😉
| /** @var bool */ | ||
| private $disableAccessTokenSaving; | ||
|
|
||
| public function __construct(EntityManagerInterface $entityManager, bool $disableAccessTokenSaving) |
There was a problem hiding this comment.
WDYT about a NullAccessTokenManager instead?
|
@Orkin are you still interested in finishing this PR? If you want someone to take over, just tell us. |
|
Hi, I would like to finish but currently don't have time so if someone want to continue I'll appreciate. Sorry for that. |
|
No worry, thanks for the quick reply. Please keep this open. |
Orkin
force-pushed
the
disable-access-token-saving
branch
6 times, most recently
from
172f5f9 to
0a9bded
Compare
| /** @var bool */ | ||
| private $persistAccessToken; | ||
|
|
||
| public function __construct(bool $persistAccessToken) |
There was a problem hiding this comment.
Can we add a NullAccessTokenManager instead?
Orkin
force-pushed
the
disable-access-token-saving
branch
from
e998f57 to
156d13d
Compare
chalasr
force-pushed
the
disable-access-token-saving
branch
from
156d13d to
03e815d
Compare
|
Thank you @Orkin. |
@mtarld @chalasr this is the port of disabled access token saving feature 😉