Skip to content

Conversation

@pgodel
Copy link
Contributor

@pgodel pgodel commented Aug 23, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11466
License MIT
Doc PR -

#10451 introduced the requirement of RequestContext in AssetsExtension. This is only needed when generating absolute URLs for assets. When sending emails with Twig from the CLI, the router may not be present and most times is not required.

This PR attempts to remove the hard dependency and set RequestContext if the router is present according to issue #11466.

Copy link
Member

Choose a reason for hiding this comment

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

message should be single-quoted.

@pgodel
Copy link
Contributor Author

pgodel commented Aug 26, 2014

Applied changes based on comments from @fabpot and @stof - Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

should probably be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, sorry for missing it. Done.

@pgodel pgodel force-pushed the issue_11466 branch 2 times, most recently from 5f8c884 to 8e38cbd Compare August 26, 2014 14:20
Copy link
Member

Choose a reason for hiding this comment

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

This message looks weird to me. "To ensure a URL as absolute..." -> "To generate an absolut URL for an asset, ..."

@fabpot
Copy link
Member

fabpot commented Aug 27, 2014

👍

1 similar comment
@stof
Copy link
Member

stof commented Aug 27, 2014

👍

@fabpot
Copy link
Member

fabpot commented Aug 27, 2014

Thank you @pgodel.

@fabpot fabpot merged commit 5ad4d8a into symfony:2.5 Aug 27, 2014
fabpot added a commit that referenced this pull request Aug 27, 2014
…ssetsExtension (pgodel)

This PR was merged into the 2.5 branch.

Discussion
----------

[TwigBundle] Remove hard dependency of RequestContext in AssetsExtension

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11466
| License       | MIT
| Doc PR        | -

#10451 introduced the requirement of RequestContext in AssetsExtension. This is only needed when generating absolute URLs for assets. When sending emails with Twig from the CLI, the router may not be present and most times is not required.

This PR attempts to remove the hard dependency and set RequestContext if the router is present according to issue #11466.

Commits
-------

5ad4d8a Remove hard dependency of RequestContext in AssetsExtension
@pgodel pgodel deleted the issue_11466 branch August 28, 2014 16:44
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.

4 participants