Allow scheme-relative URLs #1433

Closed
wants to merge 3 commits into
from

Projects

None yet

3 participants

@serveace
Contributor

URLs starting with "//" will choose the scheme of the current page. This allows more flexibility when loading content from CDNs.

See discussion at https://groups.google.com/forum/?fromgroups=#!topic/liftweb/4N0-Rkhb_1c .

serveace added some commits Apr 16, 2013
@serveace serveace Allow scheme-relative urls
URLs starting with "//" will choose the scheme of the current page.  This allows more flexibility when loading content from CDNs.
2b3e0b7
@serveace serveace Don't rewrite scheme-relative URLs 1ad4223
@serveace serveace commented on the diff Apr 16, 2013
web/webkit/src/main/scala/net/liftweb/http/Req.scala
@@ -544,6 +544,7 @@ object Req {
private def _fixHref(contextPath: String, v: Seq[Node], fixURL: Boolean, rewrite: Box[String => String]): Text = {
val hv = v.text
val updated = if (hv.startsWith("/") &&
+ !hv.startsWith("//") &&
@serveace
serveace Apr 16, 2013 Contributor

From what I can tell, it looks like it's necessary to run this check in both places.

@Shadowfiend
Member

Nice. Does this sit ready as far as you've been able to test, or are more changes needed to get everything working with scheme-relative URLs?

@serveace
Contributor

I don't currently have any environment to build lift, so I'll have to do that before I run a couple tests.

@Shadowfiend
Member

Ok, cool. You seem to have an application that would use this at least a little bit, so I think it'll make a good real-world test to ensure this works.

That said, you may want to merge this with lift_26_dev when testing, which is the 2.6-SNAPSHOT branch until we release Lift 2.5. That will let you publish-local from sbt and be able to depend in your app on 2.6-SNAPSHOT to use the resulting packages.

@serveace
Contributor

I was able to test this tonight.

I tested with and without a context, and with a mixture of different types of URLs.

Looks like everything worked as expected.

@Shadowfiend
Member

Ok, rock on. I'll try and build it and run it for some of my apps tonight to see if things look good there, and if so I'll merge 'er up!

@fmpwizard
Member

rebased to master

@fmpwizard fmpwizard closed this May 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment