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

Return '' on echo $this; for TwigView #61

Merged
merged 1 commit into from
Sep 20, 2017
Merged

Return '' on echo $this; for TwigView #61

merged 1 commit into from
Sep 20, 2017

Conversation

inoas
Copy link
Contributor

@inoas inoas commented Sep 20, 2017

This will fix {{ _view.assign('foo', 'bar') }} and similar uses (that are a bit wrong and should be {% _view.assign('foo', 'bar') %};
Latter will hopefully be possible/fixed in 5.x through some new twig tags. However templates then will need to be updated at some point before this hack can be removed again.

This will fix {{ _view.assign('foo', 'bar') }} and similar uses (that are a bit wrong and should be {% _view.assign('foo', 'bar') %};
Latter will hopefully be fixed in 5.x - however templates then will need to be updated at some point before this hack can be removed again.
@inoas
Copy link
Contributor Author

inoas commented Sep 20, 2017

Alternative to cakephp/cakephp#11217 (however if 11217 is merged, we should still do this here as 11217 should probably be removed at some point from the core if it gets added)

Closes #60

Copy link
Member

@dereuromark dereuromark left a comment

Choose a reason for hiding this comment

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

This is the right location. core PR should not be merged IMO.

@WyriHaximus
Copy link
Collaborator

LGTM, but we should as suggested do #62

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.

None yet

3 participants