dev/core#985 Fix trailing slash for urls on Windows

[seamuslee001]

Overview

This fixes an issue where addTrailingSlash would generate a \ instead of a '/' on Windows for URLs which is in correct

Before

Wrong trailing slash added to urls

After

correct trailing slash added

ping @demeritcowboy @eileenmcnaughton

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

This is consistent with other places we set urls with trailing slashes e.g. https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/System/Base.php#L301 https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/System/Base.php#L650 https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/System/WordPress.php#L87 https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/System/WordPress.php#L119

[eileenmcnaughton]

ah yes & code comment says

    if (!$slash) {
      // FIXME: Defaulting to backslash on windows systems can produce
      // unexpected results, esp for URL strings which should always use forward-slashes.
      // I think this fn should default to forward-slash instead.
      $slash = DIRECTORY_SEPARATOR;
    }

good to merge

[eileenmcnaughton]

@seamuslee001 why is this an rc issue though?

[seamuslee001]

@eileenmcnaughton just figured sooner the fix is in the better and its such a small change

[demeritcowboy]

Thanks. Although I didn't put up a PR initially because even though the patch is correct the url is used a lot of places, and I'd want to do something like check if there's already code like rtrim($url, DIRECTORY_SEPARATOR) that might now fail. And I think at least an install on windows from scratch would be appropriate. I'm willing to test it out, but maybe against master is better, depending on when you're planning 5.14.

[seamuslee001]

switched it to master given @demeritcowboy 's concerns keep merge on pass

[eileenmcnaughton]

Yeah - I think it didn't really 'qualify' for the rc

[eileenmcnaughton]

test this please

[demeritcowboy]

Just a followup note I've been running with it on my local with the userFrameworkResourceURL setting put back to portable and haven't noticed anything odd. Did a fresh install and looks ok. Also did a quick check for rtrim(..., DIRECTORY_SEPARATOR) and looks ok.

[eileenmcnaughton]

OK - let's merge then - it feels safe & sensible to me too & it will hit the 5.15 rc if I merge now

[eileenmcnaughton]

oh it IS merged