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

[WIP][Routing] Extended Expression Language in a routing component #10512

Conversation

Projects
None yet
5 participants
@mgiustiniani
Copy link

commented Mar 22, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR not yet
  • add tests
  • add docs
  • gather feedback for my changes

A new expression language engine in a routing component can't extended with new custom functions, in my project i need custom function in a route condition.

with this patch and a custom compilerpass in my bundle i can added my custom function (version_compare of php).
this feature work with symfony/routing >= 2.4

class OverrideServiceCompilerPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container){


        $definition = $container->getDefinition('router.default');

        $definition->addMethodCall('setExpressionLanguage',array(new Reference('examplebundle.routing_expression_language')));

    }
} 

a code of examplebundle.routing_expression_language is here:

class NewExpressionLanguage extends ExpressionLanguage
{

    protected function registerFunctions()
    {
        parent::registerFunctions();

        $this->register('compare', function ($version1, $version2, $operator) {
            return sprintf('version_compare(%s, %s, %s)', $version1, $version2, $operator);
        }, function (array $values, $version1, $version2, $operator) {
            return version_compare($version1, $version2, $operator);
        });
    }

}

@mgiustiniani mgiustiniani changed the title Extended Expression Language in a routing component [Routing]Extended Expression Language in a routing component Mar 22, 2014

@mgiustiniani mgiustiniani changed the title [Routing]Extended Expression Language in a routing component [Routing] Extended Expression Language in a routing component Mar 22, 2014

@mgiustiniani mgiustiniani changed the title [Routing] Extended Expression Language in a routing component [WIP][Routing] Extended Expression Language in a routing component Mar 22, 2014

@jakzal jakzal added the Routing label Mar 22, 2014

@lsmith77

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2014

how would one then parse out the relevant headers? ie. how would one then parse the version from a media type?

@mgiustiniani

This comment has been minimized.

Copy link
Author

commented Mar 24, 2014

a PR support only extension of expression language, after this PR you can inject an expression language service with injected services (ie. request, security.context etc...).

I need this PR for Versioning API with FOSRestBndle. with add version_compare function i can use a routing condition like this:

condition: "compare(request.attributes.get('version'),'1.1', '>')"

a request and context services is initialized by default

@merk

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2014

@lsmith77 seems like that part isnt actually a consideration here. I like the idea of being able to use version_compare in the routing definitions, but that leaves implementing parsing of version headers up to the user (which we could provide in FOSRestBundle)

@lsmith77 lsmith77 referenced this pull request Mar 26, 2014

Closed

Content negotiation #10538

@mgiustiniani

This comment has been minimized.

Copy link
Author

commented Apr 12, 2014

i created an experimental bundle for this functionality
https://github.com/mgiustiniani/RestExtraBundle

if (method_exists($this->matcher, 'setExpressionLanguage')) {
$this->matcher->setExpressionLanguage($this->expressionLanguage);
}
return $this->matcher = new $this->options['matcher_class']($this->getRouteCollection(), $this->context);

This comment has been minimized.

Copy link
@fabpot

fabpot Sep 23, 2014

Member

This looks wrong to me. It should just be return $this->matcher;.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Using method_exists() is not great, but it's probably the only way to keep BC here.

I like this PR. Besides my comment, can you add some tests to validate that the router actually works when injecting an expression language?

@mgiustiniani

This comment has been minimized.

Copy link
Author

commented Sep 23, 2014

Sure!

@mgiustiniani mgiustiniani force-pushed the mgiustiniani:feature-expressionlanguage-extension-for-routing-component branch from dd8dbcc to fd756bb Sep 23, 2014

@mgiustiniani mgiustiniani force-pushed the mgiustiniani:feature-expressionlanguage-extension-for-routing-component branch from fd756bb to 52aa876 Sep 23, 2014

@fabpot fabpot referenced this pull request Sep 23, 2014

Merged

Expression language extensibility #12006

1 of 2 tasks complete
@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Thinking about this a bit more, I think this is not enough. If several bundles wants to add functions, it won't be possible as we are using inheritance here.

Instead, I think we need to create some kind of FunctionProviders that you can register on ExpressionLanguage instances`. That's what I've done in #12006.

@fabpot fabpot closed this Sep 23, 2014

@mgiustiniani

This comment has been minimized.

Copy link
Author

commented Sep 23, 2014

Multiple ExpressionLanguage is a good option but can create conflict with multiple expression language.
Imho, i create an another expression language class (that extend original expression language) with your new features and user can choose a prefer option. is a bad idea?

fabpot added a commit that referenced this pull request Sep 28, 2014

feature #12006 Expression language extensibility (fabpot)
This PR was merged into the 2.6-dev branch.

Discussion
----------

Expression language extensibility

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10512
| License       | MIT
| Doc PR        | not yet

The way we can add functions to an ExpressionLanguage instance is by using inheritance. #10512 tries to make the expression language in the routing flexible but using inheritance won't work when several bundles want to add functions.

So, this PR takes another approach to solve the problem globally.

Todo:

 * [x] add some more tests
 * [ ] add some docs

Commits
-------

7c24188 [FrameworkBundle] added a compiler pass for expression language providers
4195a91 [Routing] added support for custom expression language functions
1a39046 [Security] added support for custom expression language functions
79bcd52 [DependencyInjection] added support for custom expression language functions
184742c [ExpressionLanguage] added ExpressionFunction and ExpressionFunctionProviderInterface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.