As per title, it looks like this used to work - but now it just bails out silently?

Thanks,

Pobster

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pobster’s picture

Would help if I attached the patch!

Pobster

pobster’s picture

Sorry, whitespace fail...

Thanks,

Pobster

Garrett Albright’s picture

But this code adds a schema to a schema-less URL. If someone is consciously using schema-less URLs, they are probably doing so for a reason and want it to remain a schema-less URL; for example, they don't want browsers to load assets via HTTP when the page is loaded via HTTPS. Do you disagree?

pobster’s picture

Yes I completely disagree, I think maybe you're not quite grasping why we use protocol-less URLs... The reason is to avoid mixed content warning messages (in case that's not obvious, where assets use differing protocols resulting in a message from the browser like ... well depending on which browser you use; "Do you want to view only the webpage content that was delivered securely?").

When an asset is protocol-less - this isn't a different/ new protocol, it simply means that the asset is available in both http AND https - it can respond to either. The "trick" if you want to look at it that way, is that omitting the protocol simply means, use whatever the current page is loading with and ... we already know that, as Drupal is delivering our page. So the patch is correct in that, it simply adds the current page protocol to the asset to match the rest of the page being loaded.

Thanks,

Pobster

Garrett Albright’s picture

I know why protocol-less URLs are used; what I don't understand is why you think it's a good idea to convert protocol-less URLs to ones with protocols. Aside from being unnecessary, since anything resembling a modern browser knows how to handle protocol-less URLs correctly, we also risk being too clever and breaking whatever the user was trying to achieve when they used a protocol-less URL in the first place. Case in point: Drupal caches the results of text format operations. So if this patch is applied, when someone visits a node on a site using HTTP, Pathologic will add http:// to schema-less URLs. Now if someone visits the same node using HTTPS, they'll be served the cached result of the previous visit and get a bunch of URLs with http://. Keeping schema-less URLs schema-less avoids this problem.

pobster’s picture

The entire point is that it doesn't matter? By using the protocol-less URL you're essentially releasing it to be as the rest of the page loads. So you're NOT converting it, it's being delivered as it's intended - AS the current page protocol?

I do agree with you regarding the cache-point - but tbh, it's currently worse to simply bail out and not do anything at all rather than make it compatible. I'm not *adding* it, I'm just making it parseable in the manner of which its intended for display. As well, pathologic allows you to convert your URLs to protocol-less URLs so if achieving this is your intention then it doesn't matter about the cache. All it currently does is to allow incorrect URLs to be displayed because one editor out of twenty thought it'd be a good idea to omit the protocol...

edit: Thinking about this more, the caching problem exists for general use of the module? There's nothing to prevent you from having the caching problem with replacing https URLs - it's the processing that matters, not the protocol itself - so yes, you'd just process to be protocol-less if mixed-content and caching is a concern.

Thanks,

Pobster

Garrett Albright’s picture

Okay, wait. I think I'm starting to understand what you're saying. You're saying that if someone links to something on "this" site using a schema-less URL, we should process it the same way as a URL with a schema, which might in fact mean converting it to a URL with a schema or a domain-less absolute path. Is this correct?

If so, then you're correct, and I apologize for not grepping what you were trying to say sooner. I'll try to get some variant of your patch into the codebase soon - want to add some corresponding tests.

pobster’s picture

Excellent thank you, that's correct. I'm happy to add some test cases for it as soon as I get a bit of spare time.

Pobster

Garrett Albright’s picture

Status: Patch (to be ported) » Needs review

Okay, I've got the code plus a test in the repo now. I went ahead and made a new release, too, since it's been a while and I can't remember why I hadn't done it earlier anymore… maybe I'm about to find out. :P Please give it a try.

And again, I apologize for being so stubborn earlier. I just wasn't quite catching what you were asking for.

Garrett Albright’s picture

Issue summary: View changes
Status: Needs review » Fixed

Marking as fixed for now. Please reopen if this is still problematic.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.