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

Feature - Expose Functions (calling laravel 4 helpers methods in view) #48

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ivannpaz

ivannpaz commented Jul 5, 2013

Regarding: #42

This allows the user to specify functions (for instance laravel helpers) that should be made available to the Twig Views.

'functions' => array(
    'base_path',
    'app_path',
    'camel_case',
)

This configuration would make these three functions available to the views as:

{{ base_path() }}
{{ app_path() }}
{{ camel_case("snake_case_incoming") }}

Also possible to specify a Closure callback directly:

'functions' => array(
    'bond' => function($name, $lastname) {
        return "My name is {$lastname}, {$name} {$lastname}...";
    },
)

And later in the view:

{{ bond("Rob", "Crowe") }}

For a dramatic "My name is Crowe, Rob Crowe...".

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jul 10, 2013

Collaborator

Looks nice. Maybe include the URL helpers (action, url, asset etc) by default in the config?

Collaborator

barryvdh commented Jul 10, 2013

Looks nice. Maybe include the URL helpers (action, url, asset etc) by default in the config?

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 11, 2013

@barryvdh it could be done... regarding on what to offer by default I´m open to suggestions since I personally use only one or two of them and I don´t know what others would need out-of-the-box ready.

update:
Also, I´m on hold waiting for @rcrowe ´s comment on this in case he deems necesary other changes/additions as well. In the meantime I am already using this modified version (via composer override).

ivannpaz commented Jul 11, 2013

@barryvdh it could be done... regarding on what to offer by default I´m open to suggestions since I personally use only one or two of them and I don´t know what others would need out-of-the-box ready.

update:
Also, I´m on hold waiting for @rcrowe ´s comment on this in case he deems necesary other changes/additions as well. In the meantime I am already using this modified version (via composer override).

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jul 12, 2013

Collaborator

Well, I'm mostly using asset, url, action and trans, but I guess the secure version should be used as well.
Also, should you also provide this for filters? Following the table on this page: http://twig.sensiolabs.org/doc/advanced.html
Things like camel_case (and perhaps str_plural etc) should be more appropriate as filters, because they are value transformations.

Collaborator

barryvdh commented Jul 12, 2013

Well, I'm mostly using asset, url, action and trans, but I guess the secure version should be used as well.
Also, should you also provide this for filters? Following the table on this page: http://twig.sensiolabs.org/doc/advanced.html
Things like camel_case (and perhaps str_plural etc) should be more appropriate as filters, because they are value transformations.

@hardevine

This comment has been minimized.

Show comment
Hide comment
@hardevine

hardevine commented Jul 13, 2013

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 14, 2013

I updated the PR with some default helpers, mainly related to paths and urls generation which are very particular to Laravel and cannot be found in any third party Twig Extension.

Regarding the filters, IMHO I would include those (if needed) on a different pull-request as this would go off the subject in this issue.

ivannpaz commented Jul 14, 2013

I updated the PR with some default helpers, mainly related to paths and urls generation which are very particular to Laravel and cannot be found in any third party Twig Extension.

Regarding the filters, IMHO I would include those (if needed) on a different pull-request as this would go off the subject in this issue.

@rcrowe

This comment has been minimized.

Show comment
Hide comment
@rcrowe

rcrowe Jul 14, 2013

Owner

Thanks. I'll take a look at merging this tomorrow

On Monday, 15 July 2013, ivannpaz wrote:

I updated the PR with some default helpers, mainly related to paths and
urls generation which are very particular to Laravel and cannot be found in
any third party Twig Extension.

Regarding the filters, IMHO I would include those (if needed) on a
different pull-request as this would go off the subject in this issue.


Reply to this email directly or view it on GitHubhttps://github.com//pull/48#issuecomment-20945832
.

Owner

rcrowe commented Jul 14, 2013

Thanks. I'll take a look at merging this tomorrow

On Monday, 15 July 2013, ivannpaz wrote:

I updated the PR with some default helpers, mainly related to paths and
urls generation which are very particular to Laravel and cannot be found in
any third party Twig Extension.

Regarding the filters, IMHO I would include those (if needed) on a
different pull-request as this would go off the subject in this issue.


Reply to this email directly or view it on GitHubhttps://github.com//pull/48#issuecomment-20945832
.

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 14, 2013

@rcrowe on a second look now, I believe there will be some clash regarding the default helpers added this way and the Alias loader? Namely:

'alias_shortcuts' => array(
    'config'    => 'config_get',
    'lang'      => 'lang_get',
    'logged_in' => 'auth_check',
    'url'       => 'url_to', // Particularly this one
),

I don´t know how you'd want to deal with this one, since I haven't used the AliasLoader so far.

ivannpaz commented Jul 14, 2013

@rcrowe on a second look now, I believe there will be some clash regarding the default helpers added this way and the Alias loader? Namely:

'alias_shortcuts' => array(
    'config'    => 'config_get',
    'lang'      => 'lang_get',
    'logged_in' => 'auth_check',
    'url'       => 'url_to', // Particularly this one
),

I don´t know how you'd want to deal with this one, since I haven't used the AliasLoader so far.

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jul 15, 2013

Collaborator

I would remove the url alias, als the url() helper calls URL::to() anyways.
Furthermore, I would suggest adding route, trans and trans_choice helpers. And would you ever need the path helpers in you view?

Regarding the filters, the reason I mentioned it is it works about 99% the same way as the function, except a Twig_SimpleFilter is added instead of Twig_SimpleFunction. And then tests work in the same way with Twig_SimpleTest. But I could also create a seperate PR for that functionality offcourse.

Collaborator

barryvdh commented Jul 15, 2013

I would remove the url alias, als the url() helper calls URL::to() anyways.
Furthermore, I would suggest adding route, trans and trans_choice helpers. And would you ever need the path helpers in you view?

Regarding the filters, the reason I mentioned it is it works about 99% the same way as the function, except a Twig_SimpleFilter is added instead of Twig_SimpleFunction. And then tests work in the same way with Twig_SimpleTest. But I could also create a seperate PR for that functionality offcourse.

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 15, 2013

Updated PR with @barryvdh suggestions.

ivannpaz commented Jul 15, 2013

Updated PR with @barryvdh suggestions.

@rcrowe

This comment has been minimized.

Show comment
Hide comment
@rcrowe

rcrowe Jul 15, 2013

Owner

For the sake of discussion, would it be best to add the functions support as an extension much like AliasLoader?

You've essentially done this but put it into TwigBridge.php its self.

Then I was also thinking of splitting this up into a Laravel helpers extension and a user functions extension. The reasoning mainly being that down the line when Laravel adds/removes a function it will be easier for users to pull in these changes. Instead of editing/publishing a config file (conflicts etc) it would just be a composer update away.

This is just me thinking out loud so would be great to get feedback on this.

Owner

rcrowe commented Jul 15, 2013

For the sake of discussion, would it be best to add the functions support as an extension much like AliasLoader?

You've essentially done this but put it into TwigBridge.php its self.

Then I was also thinking of splitting this up into a Laravel helpers extension and a user functions extension. The reasoning mainly being that down the line when Laravel adds/removes a function it will be easier for users to pull in these changes. Instead of editing/publishing a config file (conflicts etc) it would just be a composer update away.

This is just me thinking out loud so would be great to get feedback on this.

@hardevine

This comment has been minimized.

Show comment
Hide comment
@hardevine

hardevine commented Jul 16, 2013

@rcrowe +1

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jul 16, 2013

Collaborator

But you would lose flexibility. They have to wait before you update this package and they cannot remove the helpers without modifying the core/disabling the extension.
Don't know if this should be a problem, but I think that changing the name of a helper is pretty easy, and they should need to change it if they use it in their views. (That's why they wouldn't change soon anyways)

Collaborator

barryvdh commented Jul 16, 2013

But you would lose flexibility. They have to wait before you update this package and they cannot remove the helpers without modifying the core/disabling the extension.
Don't know if this should be a problem, but I think that changing the name of a helper is pretty easy, and they should need to change it if they use it in their views. (That's why they wouldn't change soon anyways)

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 16, 2013

@rcrowe I would really much preffer to edit a config file for conflicting methods, than having to update an extension (which could bring other incompatibility issues with it). The same as I would prefer to offer no laravel helper by default on the config (which I did, because I understand by your suggestions that this is a time saver after all. Even if they are not all used in the end).

This said, I totally agree with @barryvdh's comments on this issue.

ivannpaz commented Jul 16, 2013

@rcrowe I would really much preffer to edit a config file for conflicting methods, than having to update an extension (which could bring other incompatibility issues with it). The same as I would prefer to offer no laravel helper by default on the config (which I did, because I understand by your suggestions that this is a time saver after all. Even if they are not all used in the end).

This said, I totally agree with @barryvdh's comments on this issue.

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Jul 31, 2013

Collaborator

So, any updates on this? Should a new PR with this functionality as an extension be submitted?

Collaborator

barryvdh commented Jul 31, 2013

So, any updates on this? Should a new PR with this functionality as an extension be submitted?

@rcrowe

This comment has been minimized.

Show comment
Hide comment
@rcrowe

rcrowe Jul 31, 2013

Owner

Sorry for being slow for getting back on this.

My current stance is that I won't include the logic for loading functions into TwigBridge.php, what happens when people want filters, etc. (I agree that functions like camel_case should be a filter).

I think it would be much better for this to be in its own extension. Extensions are passed an instance
of Illuminate\Foundation\Application so can access the config file. Then it's just a case of returning the functions in the Twig function getFunctions.


Something else I had thought about is catching all defined functions.

Just like the AliasLoader registers registerUndefinedFunctionCallback, this can also be used to catch the functions that we have been talking about in this pull request. So you would instead do:

if (function_exists($name)) {
    return new Twig_SimpleFunction('name', $name);
}

That way we catch all Laravel helpers by default, even if things change as well as any user defined functions. You would then be able to alias function names to a different names if you so choose.


Suggestions (also the order loaded/checked):

  1. Move user defined functions to an extension. You can define or override any function you choose.
  2. Create a Laravel extension that would provide functions, globals & filters
  3. Fall back to using registerUndefinedFunctionCallback to check function_exists
Owner

rcrowe commented Jul 31, 2013

Sorry for being slow for getting back on this.

My current stance is that I won't include the logic for loading functions into TwigBridge.php, what happens when people want filters, etc. (I agree that functions like camel_case should be a filter).

I think it would be much better for this to be in its own extension. Extensions are passed an instance
of Illuminate\Foundation\Application so can access the config file. Then it's just a case of returning the functions in the Twig function getFunctions.


Something else I had thought about is catching all defined functions.

Just like the AliasLoader registers registerUndefinedFunctionCallback, this can also be used to catch the functions that we have been talking about in this pull request. So you would instead do:

if (function_exists($name)) {
    return new Twig_SimpleFunction('name', $name);
}

That way we catch all Laravel helpers by default, even if things change as well as any user defined functions. You would then be able to alias function names to a different names if you so choose.


Suggestions (also the order loaded/checked):

  1. Move user defined functions to an extension. You can define or override any function you choose.
  2. Create a Laravel extension that would provide functions, globals & filters
  3. Fall back to using registerUndefinedFunctionCallback to check function_exists
@rcrowe

This comment has been minimized.

Show comment
Hide comment
@rcrowe

rcrowe Jul 31, 2013

Owner

Laravel is pretty stable now (a published roadmap), so helpers shouldn't just disappear and even if they do we have plenty of time to catch them before a release is made.

If user defined functions take precedence, a composer update is a much better upgrade root for people than having to get everyone to edit a config file.

We can offer a better integration offering not just functions but also things such as filters.


I'm happy to go with what the people want though, so if you just want the config file to define which functions are available then I'm happy to go with that as long as we move that out into an extension.

Owner

rcrowe commented Jul 31, 2013

Laravel is pretty stable now (a published roadmap), so helpers shouldn't just disappear and even if they do we have plenty of time to catch them before a release is made.

If user defined functions take precedence, a composer update is a much better upgrade root for people than having to get everyone to edit a config file.

We can offer a better integration offering not just functions but also things such as filters.


I'm happy to go with what the people want though, so if you just want the config file to define which functions are available then I'm happy to go with that as long as we move that out into an extension.

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 31, 2013

Hi there,

I will close this PR and create the extension then instead. I will be back with this in a couple of days or so in case no one else does it first.

Just one notice about the composer update method vs editing a config file: Most people I´ve heard about using/learning laravel have hosting servers with no composer access, and I don´t know if this would make the system more or less usable for them. Then again, I really don´t know the percentage of people who actually love/can use composer for their day to day development works ;)

I'll be back with this in a few days!

ivannpaz commented Jul 31, 2013

Hi there,

I will close this PR and create the extension then instead. I will be back with this in a couple of days or so in case no one else does it first.

Just one notice about the composer update method vs editing a config file: Most people I´ve heard about using/learning laravel have hosting servers with no composer access, and I don´t know if this would make the system more or less usable for them. Then again, I really don´t know the percentage of people who actually love/can use composer for their day to day development works ;)

I'll be back with this in a few days!

@ivannpaz ivannpaz closed this Jul 31, 2013

@ivannpaz

This comment has been minimized.

Show comment
Hide comment
@ivannpaz

ivannpaz Jul 31, 2013

@rcrowe anyways I have my concerns, if you don't mind, could you elaborate on why adding functions via configuration is less desirable than adding extensions?

ivannpaz commented Jul 31, 2013

@rcrowe anyways I have my concerns, if you don't mind, could you elaborate on why adding functions via configuration is less desirable than adding extensions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment