Problem/Motivation

The new logic that checks the href to match the protocoll of the page doesn't support root-relative URL's (/sites/default/files....), which are the default and recommended as you don't need to cache by domain then.

Should check for things starting with a / and not dropping that then.

The handling of that also seems weird, if it unsets it then colorbox will do a request to domain/path/undefined, so it should probably log something and abort initialization instead of this?

Steps to reproduce

  • On a clean installation of Drupal 11, install the Colorbox module and the required library.
  • Configure the display settings for the Image field on the Article content type to use Colorbox, so the library loads when an article is loaded.
  • Create an Article. In the body field, create a link, with class="colorbox", to an image using a relative URL, e.g. starting with "/sites/default/files/..."
  • Save the article and view it.
  • Click the link. Colorbox loads with blank contents.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork colorbox-3521566

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

@berdir can you please provide a list of steps to reproduce the issue?

berdir opened merge request !50

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Created a merge request, works for us, but not sure if there's a better way to deal with this in general.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented
Issue summary: View changes

Added steps to reproduce.

  • paulmckibben committed a470a240 on 2.1.x authored by berdir 
    Issue #3521566 by berdir, paulmckibben: New protocol check for href...
paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented
Status: Needs review » Fixed

MR is merged. Thank you @berdir!

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented

I thought I clicked the boxes for issue credit, but they're not checked. Trying again.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
commented
Priority: Critical » Major

Incidentally, this should not have been a Critical bug, per the definitions. This did not bring down a site or cause a security issue, it just broke certain content. Changing to Major.

florian’s picture

florian commented

This version works well!
After module update and after clearing Drupal cache it is a must to clear browser's cache as well.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Saw the e-mail notification and came here to say that this should also work fine for protocol-relative paths as we just look for starting with /. but you edited your comment already.

That said, protocol-relative paths are discouraged and should not be used anymore, just always use https:// (or root relative paths), see https://www.designcise.com/web/tutorial/why-protocol-relative-url-are-no....

Status: Fixed » Closed (fixed)

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