[ExpressionLanguage] Create an ExpressionFunction from a PHP function name #21122

maidmaid · 2017-01-02T02:21:43Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

When we extend Expression Language, we often need to add PHP functions whose code is repetitive and redundant at the compiler/evaluator level. This PR proposes a new way more generic which allows to add a PHP function.

currently:

$el = new ExpressionLanguage();
$compiler = function ($str) {
    return sprintf('strtoupper(%s)', $str);
};
$evaluator = function ($arguments, $str) {
    return strtoupper($str);
};
$el->addFunction(new ExpressionFunction('strtoupper', $compiler, $evaluator));
$el->evaluate('strtoupper("hello")'); // return "HELLO"

with this PR:

$el->addFunction(ExpressionFunction::fromPhp('strtoupper'));
$el->evaluate('strtoupper("hello")'); // return "HELLO"

It includes PHP namespaced function:

$el->addFunction(ExpressionFunction::fromPhp('My\strtoupper', 'my_strtoupper'));
$el->evaluate('my_strtoupper("hello")'); // return "HELLO"

unkind · 2017-01-02T20:01:46Z

src/Symfony/Component/ExpressionLanguage/ExpressionPhpFunction.php

+ *
+ * @author Dany Maillard <danymaillard93b@gmail.com>
+ */
+class ExpressionPhpFunction extends ExpressionFunction


You can make simple named constructor in ExpressionFunction rather than utilizing inheritance:

ExpressionFunction::php('strtoupper')

I like this suggestion of using a factory instead of a new class

stof · 2017-01-06T15:53:36Z

Does it work for namespaced functions ? I think it does not in case they don't have the leading \ in the name (well, it works only if the compiled code is placed in the global namespace)

fabpot · 2017-01-07T22:37:41Z

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php

+     *
+     * @throws \InvalidArgumentException if given function name does not exist
+     */
+    public static function php($name)


What about a more expressive name like fromPhp?

Yes, it's better. I done.

maidmaid · 2017-01-07T23:13:39Z

@stof Indeed, for namespaced functions, it's doesn't work; a SyntaxError is thrown that said Unexpected character "\" around position 0.. To solve this, I throw an exception if given function is not in global namespace. WDYT?

maidmaid · 2017-01-07T23:27:10Z

@stof An another solution would be don't throw exception, to evaluate with short name of namespaced function and to compile with full name of namespaced function.

stof · 2017-01-09T11:08:48Z

A syntax error ? Which error are you talking about ? Are you trying to put the namespace in the ExpressionLanguage function name too ? This is not supported, as EL does not have namespaced functions.
this tells me that the factory would need to allow 2 arguments: the PHP function name, and the EL function name used to expose it.
The EL name could be optional in case the PHP function is not namespaced, reusing the PHP function name. And the factory would throw an exception if the PHP function is namespaced and the EL name is omitted.

The alternative is to decide than namespaced PHP functions are unsupported, but I would throw an exception early in this case, saying that EL function names cannot use \ in their name.

fabpot · 2017-01-09T14:04:24Z

I think supporting namespaced functions is better and having a second argument to change the function name in EL sounds like a good idea anyway, even for regular functions.

maidmaid · 2017-01-10T19:44:47Z

I done the changes that you suggested @fabpot and @stof.

fabpot · 2017-01-10T19:45:50Z

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php

+            throw new \InvalidArgumentException(sprintf(
+                'An expression function name must be defined if PHP function "%s" is in namespace.',
+                $phpFunction
+            ));


should be on one line

fabpot · 2017-01-10T19:46:22Z

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php

+        }
+
+        $reflection = new \ReflectionFunction($phpFunction);
+        if ($reflection->inNamespace() && !$expressionFunction) {


Instead of using reflection, what about just checking for \\ in the function name?

And I will move the second part !$expressionFunction first to make it faster.

Reflection is more expressive that \\ and is anyway used further.

I moved !$expressionFunction at first.

unkind · 2017-01-21T22:06:22Z

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php

+     * @throws \InvalidArgumentException if given PHP function name is in namespace
+     *                                   and expression function name is not defined
+     */
+    public static function fromPhp($phpFunction, $expressionFunction = null)


Doesn't Symfony have some convention to put factories/named constructors before the real constructor? I'm not sure, just asking.

No, __construct() is always first

I had to read the PHP doc to know what is expressionFunction. I would rename it to expressionFunctionName or something else.

For clarity, I renamed expressionFunction to expressionFunctionName and phpFunction to phpFunctionName.

lyrixx · 2017-01-22T02:10:06Z

@maidmaid could you update the PR description to match the new API?

maidmaid · 2017-01-24T21:00:18Z

@lyrixx Yes, I updated the PR description now.

nicolas-grekas · 2017-02-14T14:09:37Z

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php

+
+        $reflection = new \ReflectionFunction($phpFunctionName);
+        if (!$expressionFunctionName && $reflection->inNamespace()) {
+            throw new \InvalidArgumentException(sprintf('An expression function name must be defined if PHP function "%s" is in namespace.', $phpFunctionName));


An expression function name must be defined when PHP function "%s" is namespaced.?

nicolas-grekas

👍 with minor comment

maidmaid · 2017-02-16T10:02:31Z

It's done @nicolas-grekas :)

fabpot · 2017-02-16T11:32:49Z

Apparently, tests are broken when applying these changes.

maidmaid · 2017-02-19T12:53:32Z

I fixed tests @fabpot.

fabpot · 2017-02-19T22:34:29Z

@maidmaid I'm still not convinced that using reflection is needed. Here is my patch to get rid of it:

diff --git a/src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php b/src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php
index d011854804..87a494f82d 100644
--- a/src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php
+++ b/src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php
@@ -77,18 +77,16 @@ class ExpressionFunction
      */
     public static function fromPhp($phpFunctionName, $expressionFunctionName = null)
     {
+        $phpFunctionName = ltrim($phpFunctionName, '\\');
         if (!function_exists($phpFunctionName)) {
             throw new \InvalidArgumentException(sprintf('PHP function "%s" does not exist.', $phpFunctionName));
         }

-        $reflection = new \ReflectionFunction($phpFunctionName);
-        if (!$expressionFunctionName && $reflection->inNamespace()) {
+        $parts = explode('\\', $phpFunctionName);
+        if (!$expressionFunctionName && count($parts) > 1) {
             throw new \InvalidArgumentException(sprintf('An expression function name must be defined when PHP function "%s" is namespaced.', $phpFunctionName));
         }

-        $phpFunctionName = $reflection->getName();
-        $expressionFunctionName = $expressionFunctionName ?: $reflection->getShortName();
-
         $compiler = function () use ($phpFunctionName) {
             return sprintf('\%s(%s)', $phpFunctionName, implode(', ', func_get_args()));
         };
@@ -99,6 +97,6 @@ class ExpressionFunction
             return call_user_func_array($phpFunctionName, array_splice($args, 1));
         };

-        return new self($expressionFunctionName, $compiler, $evaluator);
+        return new self($expressionFunctionName ?: $parts[count($parts) - 1], $compiler, $evaluator);
     }
 }

Any downsides?

fabpot · 2017-02-20T17:22:52Z

@maidmaid Can you submit a PR on the docs to document this new feature?

fabpot · 2017-02-20T17:23:08Z

Thank you @maidmaid.

maidmaid · 2017-02-21T10:20:23Z

Thank you to you @fabpot and others reviewers. I created doc PR.

…hp (maidmaid, javiereguiluz) This PR was merged into the master branch. Discussion ---------- [ExpressionLanguage] Document ExpressionFunction::fromPhp cf symfony/symfony#21122 Commits ------- 415c4ec Update extending.rst 478400a Minor reword 059155f Add versionadded 6c9e09d Fix typo c70ea7f Fix typo 6d9af37 Add fromPhp tip

maidmaid added 2 commits January 2, 2017 02:49

Add ExpressionPhpFunction class

d9b8e9d

Add tests

8f49e9e

carsonbot added Status: Needs Review Feature labels Jan 2, 2017

maidmaid changed the title ~~El phpfn~~ [ExpressionLanguage] Add a new class ExpressionPhpFunction Jan 2, 2017

Fix cs

ec6c936

nicolas-grekas added this to the 3.x milestone Jan 2, 2017

unkind reviewed Jan 2, 2017

View reviewed changes

Use a factory instead of a new class

5ff0998

fabpot reviewed Jan 7, 2017

View reviewed changes

maidmaid added 4 commits January 7, 2017 23:59

Check namespaced functions

122dc6b

Rename factory function to fromPhp

cc13c80

Fix tests

4ba6300

Fix cs

4e40f7d

Add namespaced fn support

9f700f8

fabpot reviewed Jan 10, 2017

View reviewed changes

Fix about code review

f02bc98

maidmaid changed the title ~~[ExpressionLanguage] Add a new class ExpressionPhpFunction~~ [ExpressionLanguage] Create an ExpressionFunction from a PHP function name Jan 16, 2017

unkind reviewed Jan 21, 2017

View reviewed changes

Rename params to phpFunctionName and expressionFunctionName

ba0389a

nicolas-grekas reviewed Feb 14, 2017

View reviewed changes

nicolas-grekas approved these changes Feb 14, 2017

View reviewed changes

Improve exception msg

79883f8

Fix tests

b26fa15

Remove reflection

b11b790

fabpot closed this in 3e9b8f3 Feb 20, 2017

maidmaid mentioned this pull request Feb 21, 2017

[ExpressionLanguage] Document ExpressionFunction::fromPhp symfony/symfony-docs#7517

Merged

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ExpressionLanguage] Create an ExpressionFunction from a PHP function name #21122

[ExpressionLanguage] Create an ExpressionFunction from a PHP function name #21122

maidmaid commented Jan 2, 2017 •

edited

Loading

unkind Jan 2, 2017

nicolas-grekas Jan 6, 2017

maidmaid Jan 7, 2017

stof commented Jan 6, 2017

fabpot Jan 7, 2017

maidmaid Jan 7, 2017

maidmaid commented Jan 7, 2017 •

edited

Loading

maidmaid commented Jan 7, 2017

stof commented Jan 9, 2017

fabpot commented Jan 9, 2017

maidmaid commented Jan 10, 2017

fabpot Jan 10, 2017

maidmaid Jan 16, 2017

fabpot Jan 10, 2017

fabpot Jan 10, 2017

maidmaid Jan 16, 2017

maidmaid Jan 16, 2017

unkind Jan 21, 2017

fabpot Jan 21, 2017

lyrixx Jan 22, 2017

maidmaid Jan 24, 2017

lyrixx commented Jan 22, 2017

maidmaid commented Jan 24, 2017

nicolas-grekas Feb 14, 2017

maidmaid Feb 15, 2017

nicolas-grekas left a comment

maidmaid commented Feb 16, 2017

fabpot commented Feb 16, 2017

maidmaid commented Feb 19, 2017

fabpot commented Feb 19, 2017

fabpot commented Feb 20, 2017

fabpot commented Feb 20, 2017

maidmaid commented Feb 21, 2017

[ExpressionLanguage] Create an ExpressionFunction from a PHP function name #21122

[ExpressionLanguage] Create an ExpressionFunction from a PHP function name #21122

Conversation

maidmaid commented Jan 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maidmaid commented Jan 7, 2017 • edited Loading

maidmaid commented Jan 7, 2017

stof commented Jan 9, 2017

fabpot commented Jan 9, 2017

maidmaid commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Jan 22, 2017

maidmaid commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

maidmaid commented Feb 16, 2017

fabpot commented Feb 16, 2017

maidmaid commented Feb 19, 2017

fabpot commented Feb 19, 2017

fabpot commented Feb 20, 2017

fabpot commented Feb 20, 2017

maidmaid commented Feb 21, 2017

maidmaid commented Jan 2, 2017 •

edited

Loading

maidmaid commented Jan 7, 2017 •

edited

Loading