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

Ability to define custom functions with callback instead of class name #991

Merged
merged 1 commit into from
May 16, 2014

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Mar 27, 2014

Right now the only way to define custom DQL functions is by giving the class name, and Doctrine will create the class:

$config->addCustomNumericFunction('FOO', 'My\Custom\DqlFunction');

This is very limiting when the custom functions has dependencies, for example it can't be created by a DI container.

The approach I have taken here is very simple: it allows to define a callback instead of the class name: the callback will be called and it should return the instance:

$config->addCustomNumericFunction('FOO', function($funcName) {
    return new My\Custom\DqlFunction($funcName);
});

On a side note, I think it would be great to generalize that approach because currently there are a lot of places where the same constraints apply.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3054

We use Jira to track the state of pull requests and the versions they got
included in.

@@ -3353,7 +3353,12 @@ public function CustomFunctionsReturningNumerics()
$funcName = strtolower($this->lexer->lookahead['value']);
$funcClass = $this->em->getConfiguration()->getCustomNumericFunction($funcName);

$function = new $funcClass($funcName);
if ($funcClass instanceof \Closure) {
Copy link
Member

Choose a reason for hiding this comment

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

What about

if (is_string($functName)) {
    $function = new $funcClass($funcName);
} else {
    $function = $funcClass($funcName);
}

Also, I'd change the naming from $funcClass to $functionName

Copy link
Member

Choose a reason for hiding this comment

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

$function = is_string($functionName)
    ? new $functionClass($functionName)
    : $functionClass($functionName);

@Ocramius
Copy link
Member

@mnapoli what is the use case for having a function coming from a container?

@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 27, 2014

@Ocramius Regarding the use case, I have one with https://github.com/mnapoli/DoctrineTranslated:

SELECT e FROM MyEntity e WHERE TR(e.name) = 'hello'

Here TR is the "translation" function, which means the translation for the name must be "hello" in "the current locale". Given I use embedded objects, the TR function will turn the query into this:

SELECT e FROM MyEntity e WHERE e.name.en = 'hello'

(when the locale is en)

Have a look here: https://github.com/mnapoli/DoctrineTranslated/blob/master/src/Doctrine/TranslatedFunction.php

The test is here: https://github.com/mnapoli/DoctrineTranslated/blob/master/tests/DoctrineTest.php

The current locale must come from somewhere outside the DQL function. I don't want to get into too much details but basically the TR function will get the current locale (and the fallbacks) from the TranslationManager, which is a service.

But managing how objects are created, not just for custom functions, is a problem I regularly have. I know you have already told me (mailing list) "these should be very simple classes without dependencies" but I disagree, I have been limited by this so many times and given it can be fixed quite easily I would love to see this fixed.

@Ocramius
Copy link
Member

@mnapoli I get what you are trying to do, I'm just not sure if it is a good idea... Not really convinced. On the other side, this is indeed ok as the constructor arguments for a function are not constrained by an interface (nor will).

@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 28, 2014

I don't see a reason to put this kind of restriction. I would consider it bad practice in any library to create objects directly (I mean for extension points: objects provided by end-users) without providing an alternative.

@Ocramius
Copy link
Member

That makes a whole lot of sense :)

@mnapoli
Copy link
Contributor Author

mnapoli commented May 15, 2014

Code updated with the remarks.

Anyone wants to give feedback too? I'd love to get this going, I really need it for DoctrineTranslated.

guilhermeblanco added a commit that referenced this pull request May 16, 2014
Ability to define custom functions with callback instead of class name
@guilhermeblanco guilhermeblanco merged commit 47ca100 into doctrine:master May 16, 2014
@mnapoli
Copy link
Contributor Author

mnapoli commented May 16, 2014

\o/ thanks!

@mnapoli mnapoli deleted the custom-functions-callback branch May 16, 2014 06:32
mnapoli added a commit to mnapoli/doctrine2 that referenced this pull request May 16, 2014
@mnapoli mnapoli mentioned this pull request May 16, 2014

$function = is_string($functionClass)
? new $functionClass($functionName)
: $functionClass($functionName);
Copy link
Member

Choose a reason for hiding this comment

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

@mnapoli could you change it to call_user_func($functionClass, $functionName) to support non-closure callables on PHP 5.3 too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(fixed in #1031)

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.

5 participants