BC break: adding closure to Presenter's view() method #1881

Closed
kenjis opened this Issue Jul 3, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@kenjis
Contributor

kenjis commented Jul 3, 2015

We forgot the issue: 30643ab

On Fuel 1.7.3, "Passing functions to views" does not work.
http://fuelphp.com/docs/general/presenters.html#/functions

There is no documentaion:
https://github.com/fuel/fuel/blob/1.7/master/CHANGELOG.md#v173

@WanWizard

This comment has been minimized.

Show comment
Hide comment
@WanWizard

WanWizard Jul 3, 2015

Member

I know, but I have no decent solution as of yet, as there are two use-cases which are conflicting:

Case 1: as documented, pass a closure to be used as a modifier
Case 2: as implemented, pass a closure to be used a value provider

The only thing I can think of at the moment is to use Reflection, and use it to check if the closure has any arguments. If not, it's a case 2 (it can never be used as a modifier), and if it has, it's a case 1 (a value provider can not require arguments).

If a closure has arguments, but all are optional, it could be both, so a decision has to be made what this means (I would say that any argument present makes it a case 1).

Using reflection might have a performance impact.

Member

WanWizard commented Jul 3, 2015

I know, but I have no decent solution as of yet, as there are two use-cases which are conflicting:

Case 1: as documented, pass a closure to be used as a modifier
Case 2: as implemented, pass a closure to be used a value provider

The only thing I can think of at the moment is to use Reflection, and use it to check if the closure has any arguments. If not, it's a case 2 (it can never be used as a modifier), and if it has, it's a case 1 (a value provider can not require arguments).

If a closure has arguments, but all are optional, it could be both, so a decision has to be made what this means (I would say that any argument present makes it a case 1).

Using reflection might have a performance impact.

@kenjis

This comment has been minimized.

Show comment
Hide comment
@kenjis

kenjis Jul 3, 2015

Contributor

Why do you need case 2?

Contributor

kenjis commented Jul 3, 2015

Why do you need case 2?

@WanWizard

This comment has been minimized.

Show comment
Hide comment
@WanWizard

WanWizard Jul 3, 2015

Member

Case 2 is needed to be able to inject dynamic data retrieval or processing. It was the case that triggered the change back in january (I think).

Either way we're stuck in a double BC situation at the moment, because some will have a need for case 1 (and now have a problem) and some for case 2 (which will get into trouble if we revert it). So we do need a solution to cover both.

There's another difference (and that is the reason for the current code) is that a modifier function needs to be passed on as-is to the view (so always with encoding disabled) while a value function needs to be converted to a value first, to the encoding settings can be honoured.

Member

WanWizard commented Jul 3, 2015

Case 2 is needed to be able to inject dynamic data retrieval or processing. It was the case that triggered the change back in january (I think).

Either way we're stuck in a double BC situation at the moment, because some will have a need for case 1 (and now have a problem) and some for case 2 (which will get into trouble if we revert it). So we do need a solution to cover both.

There's another difference (and that is the reason for the current code) is that a modifier function needs to be passed on as-is to the view (so always with encoding disabled) while a value function needs to be converted to a value first, to the encoding settings can be honoured.

@WanWizard

This comment has been minimized.

Show comment
Hide comment
@WanWizard

WanWizard Jul 3, 2015

Member

We've been discussing this, and our proposal is:

We will treat a closure exactly the same way as a variable. If you pass it to a view or presenter with the filter enabled, it will be evaluated prior to being encoded. If you pass it to a view or presenter with the filter disabled (for example through set_safe), the closure will be passed to the view untouched.

For existing code, we add a BC config key, that when enabled would absorb the error when evaluating a closure meant to be a modifier, and will pass the closure without attempting to encode it. This will have a performance hit because this can only be done using reflection (to get the argument count).

Member

WanWizard commented Jul 3, 2015

We've been discussing this, and our proposal is:

We will treat a closure exactly the same way as a variable. If you pass it to a view or presenter with the filter enabled, it will be evaluated prior to being encoded. If you pass it to a view or presenter with the filter disabled (for example through set_safe), the closure will be passed to the view untouched.

For existing code, we add a BC config key, that when enabled would absorb the error when evaluating a closure meant to be a modifier, and will pass the closure without attempting to encode it. This will have a performance hit because this can only be done using reflection (to get the argument count).

@WanWizard WanWizard closed this in 627441c Jul 3, 2015

WanWizard added a commit to fuel/docs that referenced this issue Jul 3, 2015

@kenjis

This comment has been minimized.

Show comment
Hide comment
@kenjis

kenjis Jul 3, 2015

Contributor

I got it. Thanks.

Contributor

kenjis commented Jul 3, 2015

I got it. Thanks.

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