Problem/Motivation

I create some SDC components, when using theme, the js and CSS file not load correctly.
I got error in chrome console. this is the url generated by SDC:

/yiysites/core/modules/sdc/../modules%5Ccontrib%5Cblocktabs_jquery_ui%5Ccomponents%5Ctabs-default%5Ctabs-default.js?rwh76n

it should be:

/yiysites/modules/contrib/blocktabs_jquery_ui/components/tabs-default/tabs-default.js?rwh76n

Steps to reproduce

1, using a win10 OS.
2, install xampp, XAMPP is the most popular PHP development environment, https://www.apachefriends.org/index.html
3, create a site in sub directory of htdocs
4, install SDC module
5, create a component in custom module.

Proposed resolution

Windows using a different file path compare linux, it use "\\" instead of "/". we need add some logic code for windows condition.
here is some dirty code that I fixed it quickly in makePathRelativeToLibraryRoot of \Drupal\sdc\ComponentPluginManager :

	  $path_from_root=str_replace('\\','/',$path_from_root); 
    //return $dots . $path_from_root;
	$path_from_root= "/" .  $path_from_root;
	return $path_from_root;

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3367556

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:

Comments

g089h515r806 created an issue. See original summary.

dieterholvoet’s picture

Component: render system » single directory components

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

laborouge’s picture

+1 Subscribe
Same problem on Drupal 10.3.0

detroz’s picture

StatusFileSize
new703 bytes

Same problem with Laragon on Windows and Drupal 10.3.1

Here a patch tested on Drupal 10.3.1 and based on the proposed resolution by g089h515r806.

newaytech’s picture

Same issue here - manifested once I updated Bario theme - with new components added.

Path in #6 applied and fixed for me - thanks!

vensires made their first commit to this issue’s fork.

vensires changed the visibility of the branch 3367556-sdc-components-css to hidden.

vensires changed the visibility of the branch 3367556-sdc-components-css to active.

vensires’s picture

Status: Active » Needs review

I created MR!9645 and added a if {} for Windows machines specifically - same as core does in other places too.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

probably should have coverage

groendijk’s picture

@vensires Just for understanding, looking at the if (!str_starts_with(PHP_OS, 'WIN')) in core. It's always connected with a directory/file permission command. If you search for str_replace(DIRECTORY_SEPARATOR, '\\' they don't do the OS check. Is it necessary?

vensires’s picture

You are correct but here we are doing the opposite:
$path_from_root = str_replace('\\', DIRECTORY_SEPARATOR, $path_from_root);
so I think it is.

groendijk’s picture

Sounds valid, can you confirm its working?

vensires’s picture

Unfortunately no. We will need someone with Windows to be able to review it; maybe one of the previous commenters in this thread.
For now, we have the "Needs tests" tag so we are good; I don't have any experience with unit testing though and I don't know how unit testing in Linux OS could check for this bug which is targeting Windows only.

newaytech’s picture

Hi folks. I use a XAMP stack on Windows 11. Can confirm patch in #6 applied and fixed for me - do you need me to test any other patches? How do I apply the merge request to my setup - I'm using the patch via composer.json right now.

I do get an error message when restoring a production site (linux based) to my local dev - so I need to perform a drush full cache clear - otherwise I can't access the site. Not a showstopper - but just something I've never had to do before - and seems related.

Warning: file_get_contents(/etc/apache2/htdocs/drupal-8/neway-drupal-project/web/themes/contrib/bootstrap_barrio/components/alert\alert.twig): Failed to open stream: No such file or directory in Drupal\Core\Template\Loader\ComponentLoader->getSourceContext() (line 93 of core\lib\Drupal\Core\Template\Loader\ComponentLoader.php).
Drupal\Core\Template\Loader\ComponentLoader->getSourceContext('bootstrap_barrio:alert') (Line: 71)
Twig\Loader\ChainLoader->getSourceContext('bootstrap_barrio:alert') (Line: 380)
Twig\Environment->loadTemplate('__TwigTemplate_5fa659091c48341a12c1ba2127d9481d', 'bootstrap_barrio:alert') (Line: 343)
Twig\Environment->load('bootstrap_barrio:alert') (Line: 466)
Twig\Environment->resolveTemplate('bootstrap_barrio:alert') (Line: 1439)
Twig\Extension\CoreExtension::include(Object, Array, 'bootstrap_barrio:alert', Array) (Line: 45)
__TwigTemplate_bfe96b157a4c1d7d5465c2e2e9c307c6->doDisplay(Array, Array) (Line: 393)
Twig\Template->yield(Array, Array) (Line: 349)
Twig\Template->display(Array) (Line: 364)
Twig\Template->render(Array) (Line: 35)
Twig\TemplateWrapper->render(Array) (Line: 33)
twig_render_template('themes/contrib/bootstrap_barrio/templates/misc/status-messages.html.twig', Array) (Line: 348)
Drupal\Core\Theme\ThemeManager->render('status_messages', Array) (Line: 491)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 248)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 165)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 166)
Drupal\Core\Render\Renderer->renderInIsolation(Array) (Line: 191)
Drupal\Core\Render\Renderer->doRenderPlaceholder(Array) (Line: 228)
Drupal\Core\Render\Renderer->renderPlaceholder('callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&args%5B0%5D&token=_HAdUpwWmet0TOTe2PSiJuMntExoshbm1kh2wQzzzAA', Array) (Line: 697)
Drupal\big_pipe\Render\BigPipe->renderPlaceholder('callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&args%5B0%5D&token=_HAdUpwWmet0TOTe2PSiJuMntExoshbm1kh2wQzzzAA', Array) (Line: 524)
Drupal\big_pipe\Render\BigPipe->Drupal\big_pipe\Render\{closure}()
Fiber->start() (Line: 531)
Drupal\big_pipe\Render\BigPipe->sendPlaceholders(Array, Array, Object) (Line: 283)
Drupal\big_pipe\Render\BigPipe->sendContent(Object) (Line: 113)
Drupal\big_pipe\Render\BigPipeResponse->sendContent() (Line: 423)
Symfony\Component\HttpFoundation\Response->send() (Line: 20)

vensires’s picture

Thanks for volunteering @newaytech!
You need to test the diff from the MR: https://git.drupalcode.org/project/drupal/-/merge_requests/9645.diff. Use that URL instead of the patch's. As for how you may find this URL in other issues too, just search in this page for plain diff and you'll find it.

As for the restoration process, this is something I do quite often. Especially when I copy the database from a system using another URL (ex. Beta) to my own local system (ex. DDEV); I have to execute drush cr otherwise I have the same issues. I don't really think it's related to this one.

newaytech’s picture

The merge request uses a backslash "DIRECRTORY_SEPARATOR" - which renders the wrong file path to the href link.

Replacing with "/" fixes it up.

    -  $path_from_root = str_replace('\\', DIRECTORY_SEPARATOR, $path_from_root);
    + $path_from_root = str_replace('\\', "/", $path_from_root);

Are you able to re-roll as another merge request ?

vensires’s picture

It's ready @newaytech!

newaytech’s picture

StatusFileSize
new13.41 KB

yes - looks good that.

vensires’s picture

Perfect, so the major thing to decide here is what kind of tests should we expect for this small change targeting Windows-only machines...
I add the tag "Needs Review Queue Initiative" in order to get some further guidance.

marc.bau’s picture

Patch does not work properly. I still see /../ in paths.

<link rel="stylesheet" media="all" href="/web/core/../themes/contrib/bootstrap/components/menu_columns/menu_columns.css?skxqv7" />
<link rel="stylesheet" media="all" href="/web/core/../themes/contrib/bootstrap/components/menu_main/menu_main.css?skxqv7" />
danchadwick’s picture

StatusFileSize
new720 bytes

If we use DIRECTORY_SEPARATOR properly, then the code works equally well for all platforms. The previous commit had the arguments reversed. It simply normalized the separator to the expected drupal slash. Then we don't need to test for windows.

I also submit no additional tests are needed. If existing tests pass, then we haven't harmed Linux. And I can confirm that I've manually tested on Windows (WampServer, Windows 11), both by stepping through the code and by seeing it work correctly.

Patch was made on 10.3, but I expect it would apply to 11.x.

danchadwick’s picture

Priority: Normal » Major
Status: Needs work » Needs review

Changing to Major, since the site is visually unusable for all users. Might warrant Critical.

Changing to needs review since I don't believe tests are needed. I did not make a commit to the MR without approval of the other authors.

fotispan’s picture

patch #25 fixed the issue for me on a windows machine.
Thanks

vensires’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I changed the MR to match what's already proposed in #25 along with a small comment why this line of code is needed. I also I set it as RTBC based on #27, since it's decided we won't need tests.

Merry Christmas to everyone :D

jaroslav červený’s picture

I doing my first SCD commponent in custom theme on WAMP sever.
Aftter apply patch #25, I get in source code:

<link rel="stylesheet" media="all" href="/core/../themes/custom/holy/components/menu/menu.css?sqbxre" />

It works.

  • catch committed 74c4c0ab on 11.x
    Issue #3367556 by vensires, danchadwick, detroz, g089h515r806, groendijk...

  • catch committed d60b4ff7 on 10.5.x
    Issue #3367556 by vensires, danchadwick, detroz, g089h515r806, groendijk...

  • catch committed 404e24a2 on 11.1.x
    Issue #3367556 by vensires, danchadwick, detroz, g089h515r806, groendijk...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, 11.1.x and 10.5.x, thanks!

Status: Fixed » Closed (fixed)

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