Problem/Motivation

Split from #3027639: Make css/js optimized assets path configurable

The public stream wrapper has some logic to magically determine the correct base path for files, we didn't copy that to the assets stream wrapper but it should probably be as close as possible.

This broke multisite installs without a configured file_public_path.

Steps to reproduce

1. Download Drupal 10.1 via composer
2. Create docroot/sites/www folder and copy docroot/sites/default files to new folder.
3. Update docroot/sites/sites.php to point to docroot/sites/www by default.
4. Install Drupal using standard profile.
5. See JS/CSS aggregated files not rendering.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3354090.patch2.22 KBcatch
#8 3354090.patch1.94 KBcatch
#8 3354090-test-only.patch1.39 KBcatch
#6 3354090_0.patch565 bytescatch
#2 3354090.patch828 bytescatch
#2 3354090.patch828 bytescatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Category: Bug report » Task
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3354090.patch828 bytes
new 3354090.patch828 bytes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Don't mind marking this for ya.

Just for my own clarity it's fine to remove getType without a change record since it was just recently added to 10.1 which hasn't been released yet?

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Oops actually that bit's for #3354091: Asset stream wrapper may need to be selected in the UI for some cases although not the right change for there either, need to drop that hunk from the patch here.

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, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Category: Task » Bug report
Priority: Normal » Critical
Status: Needs work » Needs review
StatusFileSize
new 3354090_0.patch565 bytes

#3365739: JS/CSS assets 404 by default on multisites.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3365739: JS/CSS assets 404 by default on multisites

If possible this could do with test coverage from the steps to reproduce in #3365739: JS/CSS assets 404 by default on multisites

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3354090-test-only.patch1.39 KB
new 3354090.patch1.94 KB

Might have a test that fails - looks like setting file_public_path to NULL is the way to do it.

The last submitted patch, 8: 3354090-test-only.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented 
1) Drupal\FunctionalTests\Asset\AssetOptimizationTest::testAssetAggregationNoPublicPath
Behat\Mink\Exception\ExpectationException: Current response status code is 404, but 200 expected.

That's what we want from the failing patch.

Remaining question for me, potentially for a follow-up but could also just do here:

1. Should we merge the two test methods back into one to avoid the extra install?

2. If not #1, should we use a data provider instead of a protected helper?

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

catch credited mas5d2.

catch’s picture

catch
he/him
Primary language English
commented
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think merging them is fine, if we add a comment to mention why we need to reset file_public_path here?

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3354090.patch2.22 KB

A bit short of time, but here's a merged test. Left the helper in because it's simpler, but one test method now.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

I think this is fine.

  • larowlan committed d238872a on 10.1.x 
    Issue #3354090 by catch, longwave, smustgrave, mas5d2: Better default...

  • larowlan committed 132fa01c on 11.x 
    Issue #3354090 by catch, longwave, smustgrave, mas5d2: Better default...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and backported to 10.1.x

Status: Fixed » Closed (fixed)

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