`BASE_URL` needs replacing with `WAGTAILADMIN_BASE_URL` when using `wagtail>=3.0`

[kbayliss]

wagtail-grapple currently relys on a wagtail setting called BASE_URL, which is referenced in multiple places: https://github.com/torchbox/wagtail-grapple/search?q=BASE_URL

However, in wagtail==3.0 this is renamed to WAGTAILADMIN_BASE_URL.

From https://github.com/wagtail/wagtail/releases/tag/v3.0:

Rename the setting BASE_URL (undocumented) to WAGTAILADMIN_BASE_URL and add to documentation, BASE_URL will be removed in a future release.

[dopry]

I'm not sure that we necessarily want to replace all of those with WAGTAIL_ADMIN_BASEURL. That isn't necessarily the base URL for the site or the media.

• grapple.utils.get_media_item_url

  Media is not always hosted with the wagtail site. Most large sites I've worked on use cloud storage such as S3 and/or a Media CDN.

  Media.url calls through AstractMedia.file -> FileField.url -> Storage.url

  django.core.files.storage.FileSystemStorage

  def url(self, name):
      if self.base_url is None:
          raise ValueError("This file is not accessible via a URL.")
      url = filepath_to_uri(name)
      if url is not None:
          url = url.lstrip('/')
      return urljoin(self.base_url, url)
  
  @cached_property
  def base_url(self):
      if self._base_url is not None and not self._base_url.endswith('/'):
          self._base_url += '/'
      return self._value_or_setting(self._base_url, settings.MEDIA_URL)

  I'm fairly certain the Storage implementation should be responsible for constructing a full URL here in nearly all cases, so we shouldn't need the if url[0] == "/": branch. If we did enter it we should probably be preferencing MEDIA_URL to WAGTAIL_ADMIN_BASEURL and checking the _base_url property of the relevant storage.

  I've looked at wagtail.images.models.[WagtailImage,WagtailImageRendition], wagtailmedia.models.Media, wagatil.documents.models.Document. They all serve off of file.ur with the exception of AbstractDocument.url which falls back to serving up reverse("wagtaildocs_serve"). Reverse will return a relative url, which may be the reason the base_url is being prepended in this case. I believe this is a bug in the abstract document class and it should be updated to return a fully qualified URL.

• grapple.types.streamfield.get_media_url

  This implementation of get_media_url is used solely with embeds. As far as I can tell the embed block doesn't support relative URLs. Why would we try to alter the URL, in this case, to point to the site base URL? I could see a use case if someone was running a local embed provider. In the embedding local content case, I would recommend implementing the finder to store a relative URL with a configurable base URL is a user wanted to support being able to change the base URL of their embeds using the configuration of the finder to manage it... I think that is much more reasonable than putting a special case in wagtail grapple that would potentially mishandle a specific embed.

• grapple.types.redirects.RedirectType.resolve_old_url

  I think this one is a valid update. would something like the following be an acceptable update?

  def resolve_old_url(self, info, **kwargs):
      if settings.WAGTAILADMIN_BASE_URL:
          """
          If the WAGTAILADMIN_BASE_URL is set, we use that as the site url.
          This is only required for Wagtail >3.0
          """
          return settings.WAGTAILADMIN_BASE_URL + self.old_path
      elif settings.BASE_URL:
          """
          If the WAGTAILADMIN_BASE_URL is not set, we use the BASE_URL.
          BASE_URL is deprecated in Wagtail 3.0 and this condition should 
          be removed when dropping support for Wagtail <3.0
          """
          return settings.BASE_URL + self.old_path
      else:
          return self.old_path

[kbayliss]

@dopry Good thinking. Would that be better as a separate issue?

This issue is to address the small change required to support wagtail>=3.0, where BASE_URL has simply been renamed to WAGTAILADMIN_BASE_URL. Changing references from settings.BASE_URL to settings.WAGTAILADMIN_BASE_URL (when using wagtail>=3.0) doesn't alter any existing behaviour/functionality.

However, improving the URL handling across the project - as you've suggested - sounds sensible to me!

[dopry]

3.0 will still work with BASE_URL it's just deprecated. Removing the use of BASE_URL eliminates the need for this task and churn in the code. I opened a new ticket for the URL generation bugs.

[kbayliss]

If a project is using 3.0, they will likely have already removed BASE_URL as per the upgrade recommendations and replaced it with WAGTAILADMIN_BASE_URL, which will break wagtail-grapple unless they do something like:

BASE_URL = WAGTAILADMIN_BASE_URL = "http://..." which feels unnecessary.

Unless I'm missing something 🤔 ?

[dopry]

While it probably isn't ideal, I think having the weird config helps surface to developers we need to address url handling in grapple.

[kbayliss]

This is likely to cause further confusion, as Wagtail 5.0 states BASE_URL is no longer recognised and should be removed: https://docs.wagtail.org/en/latest/releases/5.0.html

(Speaking from experience, as I've just spent hours trying to work out why fetching images stopped working after upgrading to Wagtail 5.1 😅)

[dopry]

I feel like the wagtail documentation should maybe clarify that core no longer depends on BASE_URL and that contributed and 3rd Party modules may need it. Has there been any changes in core WRT to base urls for media?