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

also provide request itself in expressions #144

Merged
merged 1 commit into from
Aug 19, 2014
Merged

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Aug 19, 2014

follow up of #119

@ddeboer
Copy link
Member

ddeboer commented Aug 19, 2014

👍 Do we need extend our tests for this?

@ddeboer ddeboer mentioned this pull request Aug 19, 2014
6 tasks
@dbu
Copy link
Contributor Author

dbu commented Aug 19, 2014

hm, not so much that can go wrong, but we should nonetheless. added a note to #120 for now.

@dbu dbu mentioned this pull request Aug 19, 2014
@dbu
Copy link
Contributor Author

dbu commented Aug 19, 2014

are you ok to merge this without the test and do that later?

ddeboer added a commit that referenced this pull request Aug 19, 2014
also provide request itself in expressions
@ddeboer ddeboer merged commit 6eb7222 into master Aug 19, 2014
@ddeboer
Copy link
Member

ddeboer commented Aug 19, 2014

Yep!

@ddeboer ddeboer deleted the expression-request branch August 19, 2014 09:56
$values = $request->attributes->all();
// if there is an attribute called "request", it needs to be accessed through the request.
$values['request'] = $request;
$expressionLanguage = $this->getExpressionLanguage();
Copy link
Member

Choose a reason for hiding this comment

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

this means that you now require the ExpressionLanguage to be available even when no route is using it.

given the private getter caches it anyway, it does not have much value to use a local variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch, that was not intended!

will fix that after lunch

Copy link
Member

Choose a reason for hiding this comment

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

Omg @stof, you’re everywhere, thanks for the catch! 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #145

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

Successfully merging this pull request may close these issues.

None yet

4 participants