Now when you put amp query string in node with amp view mode enable, the function that controls if request is amp request or not doesn't work properly.

Attach the patch that solve it.

CommentFileSizeAuthor
#14 2711837-fix-is-amp-request-14.patch1.81 KBjansete
#13 backtrace.png132.73 KBmbrc
#7 2711837-simplify-amp-is-amp-request.patch1.28 KBDave Reid
#4 2711837-simplify-amp-is-amp-request.patch1.59 KBDave Reid
amp-fix-is-amp-request.patch1.04 KBjansete
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jansete created an issue. See original summary.

jansete’s picture

jansete CreditAttribution: jansete commented
Status: Active » Needs review
dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario as a volunteer and at Unic commented
Status: Needs review » Reviewed & tested by the community

I applied that patch on my local instance and can confirm that the patch is working. That function looks much cleaner to me now.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented
Status: Reviewed & tested by the community » Needs review
FileSize
2711837-simplify-amp-is-amp-request.patch1.59 KB

I think this could be simplified even further, and statically cache the result as well.

jansete’s picture

jansete CreditAttribution: jansete commented

I only would add the path checking, because node/%node/edit and other paths would be pass the conditions without the path checking.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Needs review » Needs work

I tested this locally, and it does work, but yes if for some reason there was a node/%node/edit?amp URL, this would return TRUE and result in theme switching. Would be good to avoid that.

This may be a common pattern for many, but this was new to me:

$result = &drupal_static(__FUNCTION);

A one-line code comment above stating that this caches the result of this function for a page request would be helpful.

$result &= $node && in_array($node->type, amp_get_enabled_types());

The &= was new syntax for me. I'm fine with it, but a comment line explaining that $result will change to FALSE if $node is NULL or if the node type is not in the array of enabled types, something along those lines, might be helpful.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented
Status: Needs work » Needs review
FileSize
2711837-simplify-amp-is-amp-request.patch1.28 KB

I simplifed some the logic with additional commands and clearer flow, and also added a check to ensure the current path matches the entity's (node's) public URI by comparing entity_uri() with current_path(). This will prevent it from matching on subpaths of the node, like the edit page.

I'm going to respectfully disagree about documenting the use of drupal_static(), it's such a common pattern in Drupal 7, and it's essentially the same as using static $result which doesn't need a comment to know that you want it declared only once no matter how many times the function is called.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Needs review » Reviewed & tested by the community

Tested this, nice fixes. Made a few minor comma changes in the code comments. Looks good.

  • mdrummond committed d832ff6 on 7.x-1.x authored by Dave Reid 
    Issue #2711837 by Dave Reid, jansete, mdrummond: Simplify is_amp_request...
RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Assigned: jansete » Unassigned
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jansete’s picture

jansete CreditAttribution: jansete commented

Use theme() functions in hook_custom_theme initialize default theme before AMP theme is asigned.
Imagine you get $node object throught menu_get_item, menu_get_object or node_load, and this node has a media content with media module, when node object loaded will use theme() functions, this could fail.

mbrc’s picture

mbrc CreditAttribution: mbrc at Unic commented
FileSize
backtrace.png132.73 KB

We're experiencing the same issue as described by jansete in #12. In our case we have a node which is displayed using AMP and contains a WYSIWYG field. Drupal will replace/render media tokens in this field while loading the node.

Setting a breakpoint in theme() yields the following backtrace:

As jansete notes, at this point in the execution the theme hasn't been initialized yet.

jansete’s picture

jansete CreditAttribution: jansete commented
FileSize
2711837-fix-is-amp-request-14.patch1.81 KB

Hi mbrc, please try this patch, is a lite node composition and checking. It faster than the current code too.

mdrummond, Dave Reid please check the patch when you can.

Greetings.

dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario as a volunteer and at Unic commented

I wonder if we should open a new issue for that or if we can reopen this one. The problem mentioned in #12 and #13 is really problematic for us. @Dave Reid is there any chance to reopen that issue?

dermario’s picture

dermario
Primary language German
Location Zürich
CreditAttribution: dermario as a volunteer and at Unic commented

@mbrc @jansete i opened #2827053: amp_is_amp_request() can use a theme before it is set as follow up.