Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use requestStack instead of request #126

Merged
merged 5 commits into from
Jan 21, 2016
Merged

Conversation

tcardonne
Copy link
Contributor

Hello,

With Symfony 3.0(.1), the Request service has been once for all removed from the service container. This service has been replaced by the request_stack service.

The problem was that the JWTManager is relying on the request service and as this service has been removed...

I don't think it's possible to keep a backward-comptability with SF < 2.4 (ie: SF 2.3, which is a LTS ending in May 2016 ...), so maybe creating a tag could be a workaround?

I remain at your disposal for any further modifications

Regards,

PS : Sorry for the double PR, I made mistakes trying to correct a comment type ...

@ghost
Copy link

ghost commented Jan 17, 2016

👍 +1

@slashfan
Copy link
Contributor

Hi, thanks for the PR !

Would you or anyone be willing to work on a "hack" to try to make it both symfony 2.3+ and 3.0+ compatible before merging ? Like we did with the security storage / security context deprecation ?

I'd like to keep the 1.* branch alive until symfony 2.3 end of life in may. The 2.0 branch will arrive after that, symfony 2.7+ and 3.0+ compatible, allowing to make a big refactoring and cleanup of the code as well as new features.

@tcardonne
Copy link
Contributor Author

Just pushed the "hack" ;)

throw new \InvalidArgumentException('Argument 1 should be an instance of Symfony\Component\HttpFoundation\RequestStack or Symfony\Component\HttpFoundation\Request');
} else {
if($requestStack instanceof RequestStack) {
$this->request = $requestStack instanceof RequestStack ? $requestStack->getCurrentRequest() : $requestStack;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ($requestStack instanceof Request) {
    $this->request = $requestStack;
} elseif ($requestStack instanceof RequestStack) {
    $this->request = $requestStack->getCurrentRequest();
} else {
    throw new \InvalidArgumentException('Argument 1 should be an instance of Symfony\Component\HttpFoundation\RequestStack or Symfony\Component\HttpFoundation\Request');
}

@Spomky
Copy link
Contributor

Spomky commented Jan 21, 2016

A better way could be to use a compiler pass.

@tcardonne
Copy link
Contributor Author

Done

But I don't know how to "to use a compiler pass". I just did what @slashfan said (do something like it has been done with the token_storage service.

Regards,

@Spomky
Copy link
Contributor

Spomky commented Jan 21, 2016

IMHO, the token_storage should also be injected with a compiler pass, but it is another topic.

1: Create the compiler pass in folder DependencyInjection\Compiler\RequestCompilerPass.php:

<?php

namespace Lexik\Bundle\JWTAuthenticationBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;

final class RequestCompilerPass implements CompilerPassInterface
{
    /**
     * {@inheritdoc}
     */
    public function process(ContainerBuilder $container)
    {
        if (!$container->hasDefinition('lexik_jwt_authentication.jwt_manager')) {
            return;
        }

        $definition = $container->getDefinition('lexik_jwt_authentication.jwt_manager');

        if ($container->hasDefinition('request_stack')) {
            $definition->addMethodCall('setRequest', array(new Reference('request_stack')));
        } else {
            $definition->addMethodCall('setRequest', array(new Reference('request')));
        }
    }
}

2: Enable it in the bundle Lexik\Bundle\JWTAuthenticationBundle\LexikJWTAuthenticationBundle:

...
use Lexik\Bundle\JWTAuthenticationBundle\DependencyInjection\Compiler\RequestCompilerPass;
...
class LexikJWTAuthenticationBundle extends Bundle
{
    ...
    public function build(ContainerBuilder $container)
    {
        parent::build($container);
        ...
        $container->addCompilerPass(new RequestCompilerPass());
    }
...

3: Remove the method call in the service definition

<call method="setRequest">
    <argument /> <!-- request_stack or request for Symfony <2.4 -->
</call>

@Spomky
Copy link
Contributor

Spomky commented Jan 21, 2016

I think the composer.json file should be updated too as tests did not passed because of missing components:

"require": {
    "php": ">=5.4.8",
    "symfony/framework-bundle": "~2.3|~3.0",
    "symfony/security-bundle": "~2.3|~3.0",
    "symfony/console": "~2.3|~3.0",
    "namshi/jose": "~6.0"
},

Plus PHPUnit 5 could be used:

"require-dev": {
    "phpunit/phpunit": "^4.1|^5.0",
    "symfony/phpunit-bridge": "~2.7|~3.0"
}

@tcardonne
Copy link
Contributor Author

Thanks. It's done 👍

@slashfan
Copy link
Contributor

Great work guys !

slashfan added a commit that referenced this pull request Jan 21, 2016
Use requestStack instead of request
@slashfan slashfan merged commit 2e28704 into lexik:master Jan 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants