Problem/Motivation
When aggregation is enabled on Drupal 10.1 while using AMP, css/ assets will not be generated, this is because Drupal has changed its aggregation method to be loaded using a controller, and if the optimized files are not found, they are generated, but AMP prevents this process from happening.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3374942-16.patch | 1.17 KB | ammar_ar |
| #2 | amp_prevents_css_assets_from_generating_3374942_02.patch | 1.17 KB | ahmad smhan |
Issue fork amp-3374942
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
Comment #2
ahmad smhan commentedComment #3
hardikpandya commentedI have tried this patch on one of my projects and it works. I am leaving this for others review and to be marked as RTBC.
Comment #4
marcoscanoI feel we need to come up with better wording for this, and maybe use a watchdog message instead of a message to the user?
In any case, the patch in #2 works for us when aggregation is enabled. I'm going to leave the ticket in "Needs review" so the maintainers can chime in on the above mentioned issue.
Thanks!
Comment #5
szato commentedThe same issue on D10.1.6, patch #2 solved the issue. Thank you.
Comment #6
mcgowanm commentedIf anyone is still having issues after applying this patch and your environment has htauth on try turning it off.
Comment #9
anairamzapI've created a MR that uses the provided patch at #2 with the following changes:
$needs_rewritecheck, so we always send the assets to thedoRewrite()method. I've checked and without that, the custom fonts won't be loaded (404) since we were trying to get them using the relative one level up../. Screenshot attached trying to explain this error.I will also provide, in a separate branch another "solution" for this css not loading issue. Being more like a workaround to be able to keep using the
file_get_contentsapproach, simply ignoring aggregation for css (exactly as this module is doing with the javascript) until we can find a working solution that applies to all environments (please not the curl guzzle request will also fail if sites are behind a proxy).Comment #11
anairamzapComment #12
anairamzapok, so... I've tested https://git.drupalcode.org/project/amp/-/merge_requests/17 in one of our sites dev environment that requires to have a cookie setup in order to load the pages and the guzzle approach is failing with a 403 since that request does not have the cookie set. I imagine a similar thing would happen on sites behind a proxy and we know is also happening on a local docker environment.
I'll try to modify the MR re-writing the render method. Setting back to needs work.
Comment #13
anairamzapComment #14
nicobot commentedI can also confirm that patch fixes the problem.
I agree with #4 about wording and instead of showing this error to the user it would be nice to add it to watchdog.
The MR looks like it's doing too many things. In our infrastructure we have varnish in front and the patch worked fine. Also with local docker environments, we use DDEV.
I don't understand the use case of a required cookie to load css assets, but I leave it for maintainers to double check it.
Thanks for all the contributions!
Comment #16
ammar_ar commentedreroll patch #2 to work with 3.9.0