Needs review
Project:
Accelerated Mobile Pages (AMP)
Version:
8.x-3.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
17 Jul 2023 at 09:04 UTC
Updated:
3 Feb 2025 at 14:17 UTC
Jump to comment: Most recent, Most recent file
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