Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web Asset Manager issue loading external resource ends with / character #44759

Closed
joomdonation opened this issue Jan 20, 2025 · 4 comments · May be fixed by #44774
Closed

Web Asset Manager issue loading external resource ends with / character #44759

joomdonation opened this issue Jan 20, 2025 · 4 comments · May be fixed by #44774

Comments

@joomdonation
Copy link
Contributor

Steps to reproduce the issue

Use the code below to load Stripe JS:

$wa = Factory::getDocument()->getWebAssetManager();
$wa->registerAndUseScript('sesame.stripepay', 'https://js.stripe.com/v3/');

Expected result

Stripe JS loaded properly

Actual result

Stripe JS is not loaded. The script element has wrong src like below

<script src="/joomla/root/path/https://js.stripe.com/v3/?12f2b9" data-asset-name="sesame.stripepay"></script>

System information (as much as possible)

Additional comments

  • Remove / at the end of the URL solves the issue. The code $wa->registerAndUseScript('sesame.stripepay', 'https://js.stripe.com/v3'); works properly
  • The line of code causes bug
    // Asset for the ES modules may give us a folder for ESM import map
    if (str_ends_with($path, '/') && !str_starts_with($path, '.')) {
    $path = Uri::root(true) . '/' . $path;
    }

Ping @Fedik

@Elfangor93
Copy link
Contributor

Maybe we should check for the appearance of 'http' or 'https' in $path before changing it?

@C-Lodder
Copy link
Member

C-Lodder commented Jan 21, 2025

Could check if the $path is NOT a URL:

if (str_ends_with($path, '/') && !str_starts_with($path, '.') && !filter_var($path, FILTER_VALIDATE_URL)) { 
   $path = Uri::root(true) . '/' . $path; 
}

@Fedik
Copy link
Member

Fedik commented Jan 21, 2025

You both very close to solution, but there actualy 3-rd way :)
I will do PR later.

@Fedik
Copy link
Member

Fedik commented Jan 24, 2025

Please test #44774

@Fedik Fedik closed this as completed Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants