Closed (fixed)
Project:
Accelerated Mobile Pages (AMP)
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
22 Apr 2016 at 17:25 UTC
Updated:
14 Nov 2016 at 10:08 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
jansete commentedComment #3
dermarioI applied that patch on my local instance and can confirm that the patch is working. That function looks much cleaner to me now.
Comment #4
dave reidI think this could be simplified even further, and statically cache the result as well.
Comment #5
jansete commentedI only would add the path checking, because node/%node/edit and other paths would be pass the conditions without the path checking.
Comment #6
rainbowarrayI 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:
A one-line code comment above stating that this caches the result of this function for a page request would be helpful.
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.Comment #7
dave reidI 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 $resultwhich doesn't need a comment to know that you want it declared only once no matter how many times the function is called.Comment #8
rainbowarrayTested this, nice fixes. Made a few minor comma changes in the code comments. Looks good.
Comment #10
rainbowarrayComment #12
jansete commentedUse 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.
Comment #13
mbrc commentedWe'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.
Comment #14
jansete commentedHi 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.
Comment #15
dermarioI 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?
Comment #16
dermario@mbrc @jansete i opened #2827053: amp_is_amp_request() can use a theme before it is set as follow up.