- Issue created by @mihaic
- First commit to issue fork.
- Merge request !46Issue #3372105: CSS and JS aggregation not working on Drupal 10.1 β (Closed) created by g_miric
- last update
over 1 year ago 2 pass - Status changed to Needs review
over 1 year ago 3:40pm 5 July 2023 - π·πΈSerbia g_miric
Stage file proxy is using StageFileProxySubscriber and it doesn't check config related to excluded extensions.
Here is a MR that fixes it.Testing steps:
- Apply MR
- Set excluded extensions in the settings page (/admin/config/system/stage_file_proxy) - "js, css"
- Clear cache
- πΊπΈUnited States apotek
The fix looks good on paper and passes automated tests. What's the best way to test this?
- πΊπΈUnited States smustgrave
Just started a 2.1.x branch for php8 only need to make sure it works there first.
- πΊπΈUnited States apotek
@smustgrave Do you need this merge request to be rebased onto 2.1? The bug is being experienced in production on the 2.0 branch. So maybe we need two patches. Currently both 2.0 and 2.1 support D10, which is where the bug appears.
- πΊπΈUnited States smustgrave
Would need one for both.
But have you checked to see if this is a core issue? https://www.drupal.org/project/issues/search/drupal?text=aggregation&ass... β
- π«π·France andypost
Maybe instead of MR the module can exclude path which is
public://styles/
? - π·πΈSerbia g_miric
It isn't a core issue. I applied patch that resolves the core issue and the issue was still present.
Also, without this MR you are not able to excluded extensions. There is a config available for it, but not the code that will support it.I will rebase in on 2.1.x branch.
- π·πΈSerbia g_miric
Target branch changed from 2.0.x to 2.1.x. Please review.
- π³π±Netherlands heine
This happens on both Drupal 9 and 10. Not sure about the proposed fix as this would still proxy .css.gz and .js.gz assets.
IMO SFP should not proxy anything for assets (D10) or anything in public://css and public://js (D9).
- πΊπΈUnited States micahw156
Note: This issue seems to be mitigated by enabling the new file_assets_path setting β and moving JS/CSS aggregation files out of public://.
- π³π±Netherlands heine
Is the MR necessary? StageFileProxySubscriber could exclude paths already when initializing the AlterExcludedPathsEvent.
- π·πΈSerbia g_miric
It is necessary. It fixes another issue π Stage file proxy attempts to fetch aggregated JS/CSS. Fixed
- Status changed to Needs work
over 1 year ago 3:00pm 24 July 2023 - π³π±Netherlands heine
Could you please move inability to exclude extensions to a separate issue? Marking this needs work to exclude local CSS/JS assets. We've scheduled time tomorrow to provide a patch.
- π·πΊRussia kala4ek π·πΊ Novosibirsk
Attaching changes from MR as a patch, so it can be safely applied via composer.
- Status changed to Needs review
over 1 year ago 10:03pm 3 August 2023 - πΊπΈUnited States neclimdul Houston, TX
I agree with Heine, this this doesn't solve the problem. Even if it could solve the problems with aggregation, it has problem.
1. it requires you _want_ to exclude all css and js files. I don't have a use case but if have css files you do want to be able to stage, this would block that use case
2. it requires additional configuration to get working
3. as Heine noted, it doesn't address all use cases or possibly more complicated aggregation scenarios in the futureHere's a patch that seems to addresses excluding aggregation specifically. The assumptions in StageFileProxySubscriber around paths are a little weird and the logic for the asset path pretty complicated but this addresses it as well as imagecache(Styles) so I think its probably fine for the current state of the module.
- πΊπΈUnited States smustgrave
Actually got bit by this issue myself. Going to try out #21 but would like a 2nd plus +1 before merging if possible
- π¦πΊAustralia Nadim Hossain
I had the same issue, tested my project with this patch and it is working fine now. Once someone reviews it, I think we can move it to the next stage.
- Status changed to RTBC
over 1 year ago 3:02pm 8 August 2023 -
markdorison β
committed baeee3c2 on 2.1.x authored by
neclimdul β
Issue #3372105 by g_miric, neclimdul, kala4ek, andypost, Heine, apotek,...
-
markdorison β
committed baeee3c2 on 2.1.x authored by
neclimdul β
- Status changed to Fixed
over 1 year ago 3:04pm 8 August 2023 - πΊπΈUnited States markdorison
Committed to 2.1.x. Should this be backported to 2.0.x without the usage of
str_starts_with
? - πΊπΈUnited States smustgrave
I don't think so. The change in core only landed in 10.1 I believe.
Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
over 1 year ago 12:31pm 25 August 2023 - π³π±Netherlands nieuwkar
Hi there,
I've provided this new patch, that creates a new type of event "AlterExcludedExactPathsEvent" and runs it separately after the old event "AlterExcludedPathsEvent" in order to allow backwards compatibility. This new event disallows /js and /css paths as a root argument for the public file directory (
/js/ or
/css/).Please let me know what you make of it.
Cheers!