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:

  1. Set up AMP as per AMP's module page. (Enable module, an AMP theme, enable AMP in a content type's display.
  2. Enable the WYSIWYG Media module and set up a WYSIWYG (ckeditor) profile with the Media Browser button/plugin.
  3. Add the "Convert Media tags to markup" filter in a text format.
  4. 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.

Comments

oliverpolden created an issue. See original summary.

oliverpolden’s picture

Issue summary: View changes
oliverpolden’s picture

StatusFileSize
new3.24 KB

Patch with test that fails due to the non-amp page.

oliverpolden’s picture

StatusFileSize
new3.36 KB

Fix Coding Standards and attempt to fix fatal error.

oliverpolden’s picture

StatusFileSize
new5.53 KB

Copied getTestFile and drupalGetTestFiles from media.test.

oliverpolden’s picture

StatusFileSize
new5.54 KB
oliverpolden’s picture

StatusFileSize
new5.52 KB
oliverpolden’s picture

StatusFileSize
new5.29 KB
oliverpolden’s picture

It may not be able to update test_dependencies with a patch: https://www.drupal.org/project/drupalci_testbot/issues/2692407#comment-1...

oliverpolden’s picture

StatusFileSize
new5.53 KB
oliverpolden’s picture

StatusFileSize
new5.53 KB

oliverpolden’s picture

StatusFileSize
new3.04 KB

This is a correctly failing test due to the issue reported. Please note, the patch is #12 not #13 as this comment.

coufu’s picture

I 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.

oliverpolden’s picture

@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.

oliverpolden’s picture

Comparing 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.

oliverpolden’s picture

Issue summary: View changes
oliverpolden’s picture

StatusFileSize
new5.21 KB

Here's a possible fix. It avoids menu_get_object() by querying the database directly.

oliverpolden’s picture

Status: Active » Needs review
coufu’s picture

This patch is the bomb. Sorry I haven't had a chance to reply til today. You did a great job. Thank you!

izus’s picture

Status: Needs review » Reviewed & tested by the community

#18 works fine
Thanks

chegor’s picture

+1 RTBC