When a media item is entered using the media_wysiwyg module and the "Convert Media tags to markup" filter is enabled, the initial page load of an AMP page is not an AMP page.
I have created a test that intentionally fails to highlight this. The patch with the test can be found below or in comment #13.
Steps to replicate:
- Set up AMP as per AMP's module page. (Enable module, an AMP theme, enable AMP in a content type's display.
- Enable the WYSIWYG Media module and set up a WYSIWYG (ckeditor) profile with the Media Browser button/plugin.
- Add the "Convert Media tags to markup" filter in a text format.
- Create a content item of the type above. Save and view the AMP version or add ?amp to the URL of the page.
Expected:
An AMP themed page.
Result:
The page uses the default theme.
This can also be replicated by clearing the cache and viewing the AMP URL. If you delete the media element then the page is AMP themed successfully on initial page load.
Hypothesis:
The media element causes theme hooks to be called before $custom_theme = &drupal_static(__FUNCTION__); is set within menu_get_custom_theme(). This means that while menu_get_custom_theme() is initially called with $initialize = TRUE, the hooks end up calling menu_get_custom_theme($initialize = FALSE) which results in the default theme being used.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | amp-initial-page-load-3031306-18.patch | 5.21 KB | oliverpolden |
Comments
Comment #2
oliverpolden commentedComment #3
oliverpolden commentedPatch with test that fails due to the non-amp page.
Comment #4
oliverpolden commentedFix Coding Standards and attempt to fix fatal error.
Comment #5
oliverpolden commentedCopied getTestFile and drupalGetTestFiles from media.test.
Comment #6
oliverpolden commentedComment #7
oliverpolden commentedComment #8
oliverpolden commentedComment #9
oliverpolden commentedIt may not be able to update test_dependencies with a patch: https://www.drupal.org/project/drupalci_testbot/issues/2692407#comment-1...
Comment #10
oliverpolden commentedComment #11
oliverpolden commentedComment #13
oliverpolden commentedThis is a correctly failing test due to the issue reported. Please note, the patch is #12 not #13 as this comment.
Comment #14
coufu commentedI am experiencing a problem with Media as well. In my case, however, it is because the content type has an image field using Media. When I remove the image from that field, AMP works fine.
Thanks so much for your contribution to this oliverpolden. I would have never figured this out had you not reported it. Will try to patch and will report back.
Comment #15
oliverpolden commented@Coufu did you get anywhere with this? It may not even be directly an AMP issue and could be a Media or Core issue.
I checked this with Page Theme (https://www.drupal.org/project/page_theme) and couldn't replicate it with that.
Comment #16
oliverpolden commentedComparing the page loads:
The AMP page has this line:
$custom_themes = array_filter(module_invoke_all('custom_theme'), 'drupal_theme_access');Which ends up calling:
drupal_theme_initialize()which in turn calls$custom_theme = menu_get_custom_theme();In the Page Theme module, drupal_theme_initialize() is not invoked.
Comment #17
oliverpolden commentedComment #18
oliverpolden commentedHere's a possible fix. It avoids menu_get_object() by querying the database directly.
Comment #19
oliverpolden commentedComment #20
coufu commentedThis patch is the bomb. Sorry I haven't had a chance to reply til today. You did a great job. Thank you!
Comment #21
izus commented#18 works fine
Thanks
Comment #22
chegor commented+1 RTBC