NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

maidmaid wants to merge14 commits intosymfony:masterfrommaidmaid:el-phpfn

Closed

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

maidmaid wants to merge14 commits intosymfony:masterfrommaidmaid:el-phpfn

Conversation

Copy link

Contributor

maidmaid commentedJan 2, 2017•
edited
Loading

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 weextend 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 =newExpressionLanguage();$compiler =function ($str) {returnsprintf('strtoupper(%s)',$str);};$evaluator =function ($arguments,$str) {returnstrtoupper($str);};$el->addFunction(newExpressionFunction('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"

maidmaid added2 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 the3.x milestone

Jan 2, 2017

unkind reviewed

Jan 2, 2017

View reviewed changes

src/Symfony/Component/ExpressionLanguage/ExpressionPhpFunction.php Outdated

		*
		* @author Dany Maillard <danymaillard93b@gmail.com>
		*/
		class ExpressionPhpFunctionextends ExpressionFunction

Copy link

Contributor

unkindJan 2, 2017

Choose a reason for hiding this comment

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

You can make simple named constructor inExpressionFunction rather than utilizing inheritance:

ExpressionFunction::php('strtoupper')

Copy link

Member

nicolas-grekasJan 6, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidJan 7, 2017

Choose a reason for hiding this comment

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

Done

Copy link

Member

stof commentedJan 6, 2017

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)

Use a factory instead of a new class

5ff0998

fabpot reviewed

Jan 7, 2017

View reviewed changes

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php Outdated

		*
		* @throws \InvalidArgumentException if given function name does not exist
		*/
		publicstaticfunctionphp($name)

Copy link

Member

fabpotJan 7, 2017

Choose a reason for hiding this comment

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

What about a more expressive name likefromPhp?

Copy link

ContributorAuthor

maidmaidJan 7, 2017

Choose a reason for hiding this comment

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

Yes, it's better. I done.

maidmaid added4 commits

January 7, 2017 23:59

Check namespaced functions

122dc6b

Rename factory function to fromPhp

cc13c80

Fix tests

4ba6300

Fix cs

4e40f7d

Copy link

ContributorAuthor

maidmaid commentedJan 7, 2017•
edited
Loading

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

Copy link

ContributorAuthor

maidmaid commentedJan 7, 2017

@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.

Copy link

Member

stof commentedJan 9, 2017

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.

Copy link

Member

fabpot commentedJan 9, 2017

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.

Add namespaced fn support

9f700f8

Copy link

ContributorAuthor

maidmaid commentedJan 10, 2017

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

fabpot reviewed

Jan 10, 2017

View reviewed changes

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php Outdated

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

Copy link

Member

fabpotJan 10, 2017

Choose a reason for hiding this comment

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

should be on one line

Copy link

ContributorAuthor

maidmaidJan 16, 2017

Choose a reason for hiding this comment

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

Done.

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php Outdated

		}

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

Copy link

Member

fabpotJan 10, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

fabpotJan 10, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidJan 16, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidJan 16, 2017

Choose a reason for hiding this comment

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

I moved!$expressionFunction at first.

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

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php Outdated

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

Copy link

Contributor

unkindJan 21, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

fabpotJan 21, 2017

Choose a reason for hiding this comment

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

No,__construct() is always first

Copy link

Member

lyrixxJan 22, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidJan 24, 2017

Choose a reason for hiding this comment

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

For clarity, I renamedexpressionFunction toexpressionFunctionName andphpFunction tophpFunctionName.

Copy link

Member

lyrixx commentedJan 22, 2017

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

Rename params to phpFunctionName and expressionFunctionName

ba0389a

Copy link

ContributorAuthor

maidmaid commentedJan 24, 2017

@lyrixx Yes, I updated the PR description now.

nicolas-grekas reviewed

Feb 14, 2017

View reviewed changes

src/Symfony/Component/ExpressionLanguage/ExpressionFunction.php Outdated


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

Copy link

Member

nicolas-grekasFeb 14, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

maidmaidFeb 15, 2017

Choose a reason for hiding this comment

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

Done

nicolas-grekas approved these changes

Feb 14, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍 with minor comment

Improve exception msg

79883f8

Copy link

ContributorAuthor

maidmaid commentedFeb 16, 2017

It's done@nicolas-grekas :)

Copy link

Member

fabpot commentedFeb 16, 2017

Apparently, tests are broken when applying these changes.

Fix tests

b26fa15

Copy link

ContributorAuthor

maidmaid commentedFeb 19, 2017

I fixed tests@fabpot.

Copy link

Member

fabpot commentedFeb 19, 2017

@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.phpindex 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?

Remove reflection

b11b790

Copy link

Member

fabpot commentedFeb 20, 2017

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

Copy link

Member

fabpot commentedFeb 20, 2017

Thank you@maidmaid.

fabpot closed this in3e9b8f3

Feb 20, 2017

maidmaid mentioned this pull request

Feb 21, 2017

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

Merged

Copy link

ContributorAuthor

maidmaid commentedFeb 21, 2017

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

xabbuh added a commit to symfony/symfony-docs that referenced this pull request

Feb 28, 2017

feature#7517[ExpressionLanguage] Document ExpressionFunction::fromP…

1b4eab4

…hp (maidmaid, javiereguiluz)This PR was merged into the master branch.Discussion----------[ExpressionLanguage] Document ExpressionFunction::fromPhpcfsymfony/symfony#21122Commits-------415c4ec Update extending.rst478400a Minor reword059155f Add versionadded6c9e09d Fix typoc70ea7f Fix typo6d9af37 Add fromPhp tip

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

Labels

Feature Status: Needs Review

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

maidmaid commentedJan 2, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commentedJan 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidmaid commentedJan 7, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

maidmaid commentedJan 7, 2017

Uh oh!

stof commentedJan 9, 2017

Uh oh!

fabpot commentedJan 9, 2017

Uh oh!

maidmaid commentedJan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyrixx commentedJan 22, 2017

Uh oh!

maidmaid commentedJan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

maidmaid commentedFeb 16, 2017

Uh oh!

fabpot commentedFeb 16, 2017

Uh oh!

maidmaid commentedFeb 19, 2017

Uh oh!

fabpot commentedFeb 19, 2017

Uh oh!

fabpot commentedFeb 20, 2017

Uh oh!

fabpot commentedFeb 20, 2017

Uh oh!

maidmaid commentedFeb 21, 2017

Uh oh!

Reviewers

Assignees

Labels

maidmaid commentedJan 2, 2017•
edited
Loading

maidmaid commentedJan 7, 2017•
edited
Loading