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

Wrap relative path with asset() Twig function #825

Merged
merged 1 commit into from
Dec 3, 2016

Conversation

bocharsky-bw
Copy link
Contributor

I think it's a good idea to adjust relative path with asset() Twig function, isn't it?

@robfrawley
Copy link
Collaborator

robfrawley commented Nov 27, 2016

@bocharsky-bw So, I know the Asset Component (documentation here) was introduced in Symfony 2.7, but did this function exist prior to the component being compartmentalized into its own release?

Most importantly, was this function available in a default Symfony 2.3 installation? I honestly don't remember if it was, and I do know we want to retain explicit 2.3 support on this branch...

That said, if it did exist, let me know, otherwise, I'll merge it into the 2.x branch instead, as we are breaking BC on that branch, and I do like the overall sentiment of this change.

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Nov 27, 2016
@lsmith77
Copy link
Contributor

@robfrawley
Copy link
Collaborator

@lsmith77 Looks like asset's been around for quite some time. I assume there are no objections to utilizing this in our examples? It is better practice to make use of the helper functions in a real project anyway. I'll merge tomorrow morning unless anyone objects.

@robfrawley
Copy link
Collaborator

robfrawley commented Nov 27, 2016

@lsmith77 @antoligy

Also, I didn't intentionally turn on the Coveralls verbose comment reporting when I enabled coverage reporting (perhaps that's the new default?), and, well, I'm not quite sure I like it, either (if you have no idea what I'm talking about, check up in this discussion thread a bit ;-). Thoughts?

I'm inclined to go ahead and disable those verbose comments in Coverall's settings... The repo commit hooks should be enough (where it already checks Travis, StyleCI, and Coveralls now). We don't need strings of comments from the coveralls bot on top of that too, do we?

@lsmith77
Copy link
Contributor

@robfrawley imho its sufficient if such coverage infos can easily be seen via a link in the checks.

@bocharsky-bw
Copy link
Contributor Author

Now I'm not sure about this changes. What if assets will be hosted on a CDN? Then the asset() function will build an absolute URL for them I think. Looks like it works for relative paths only.

@lsmith77
Copy link
Contributor

the function lets you configure different CDN base urls .. but I guess for most users it makes sense to use the asset filter ..

@robfrawley robfrawley merged commit 57d533e into liip:1.0 Dec 3, 2016
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Dec 3, 2016
@alexwilson
Copy link
Collaborator

🎉

@bocharsky-bw bocharsky-bw deleted the patch-1 branch December 3, 2016 22:05
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