That's a use case that we have had, and could be useful for someone else, so we created a patch that I'll be adding in a second.

Essentially, we are adding a new checkbox next to the Published standard in all nodes to disable/unpublish amp for that page without having to unpublish the whole page

CommentFileSizeAuthor
#62 disable_amp_for-2802477-61.patch11.62 KBmarcelovani
#60 disable_amp_for-2802477-59.patch11.75 KBmarcelovani
#58 debug.patch11.91 KBmarcelovani
#56 disable_amp_for-2802477-56.patch11.76 KBmarcelovani
#54 disable_amp_for-2802477-54.patch11.75 KBmarcelovani
#52 disable_amp_for-2802477-52.patch8.88 KBmarcelovani
#47 interdiff.txt3.38 KBguardiola86
#47 disable_amp_for-2802477-46.patch8.86 KBguardiola86
#40 interdiff.txt3.38 KBguardiola86
#40 disable_amp_for-2802477-38.patch8.86 KBguardiola86
#36 disable_amp_for-2802477-36.patch7.1 KBguardiola86
#34 disable_amp_for-2802477-33.patch1.76 KBguardiola86
#32 disable_amp_for-2802477-32.patch7.1 KBguardiola86
#31 disable_amp_for-2802477-31.patch8.86 KBmarcelovani
#29 disable_amp_for-2802477-29.patch8.83 KBmarcelovani
#28 disable_amp_for-2802477-28.patch8.58 KBmarcelovani
#27 interdiff.txt11.98 KBmarcelovani
#27 disable_amp_for-2802477-27.patch6.83 KBmarcelovani
#26 amp-disable-amp-node-specific-2802477-26-7.x.patch11.55 KBmcrittenden
#24 amp-disable-amp-node-specific-2802477-24-7.x.patch11.56 KBjbloomfield
#23 interdiff.txt4.14 KBmcrittenden
#23 amp-disable-amp-node-specific-2802477-21-7.x.patch11.58 KBmcrittenden
#22 amp-disable-amp-node-specific-2802477-20-7.x.patch7.41 KBalexmoreno
#19 amp-disable-amp-node-specific-2802477-19-7.x.patch5.67 KBalexmoreno
#17 amp-disable-amp-node-specific-2802477-17-7.x.patch7.97 KBmcrittenden
#16 amp-disable-amp-node-specific-2802477-14-7.x.patch5.66 KBjpoirierpinto
#14 amp-disable-amp-node-specific-2802477-13-7.x.patch5.67 KBjpoirierpinto
#12 amp-disable-amp-node-specific-2802477-12-7.x.patch7.77 KBmcrittenden
#10 amp-disable-amp-node-specific-2802477-10-7.x.patch7.2 KBalexmoreno
#8 disable-amp-specific-nodes-2802477-7x.patch7.34 KBalexmoreno
#7 disable-amp-specific-nodes-2802477.patch7.2 KBalexmoreno
#5 disable-amp-specific-nodes-2802477.patch5.25 KBalexmoreno
#2 amp-2802477.patch2.02 KBalexmoreno
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexmoreno created an issue. See original summary.

alexmoreno’s picture

Issue summary: View changes
FileSize
2.02 KB
alexmoreno’s picture

Version: 8.x-1.0-beta3 » 7.x-1.0-beta3
Anonymous’s picture

This is a promised feature mentioned in the Readme.md file in the AMP download.

Create a way for users to identify which content types should provide AMP pages, and a way to override individual nodes to prevent them from being displayed as AMP pages (to use for odd pages that wouldn’t transform correctly).

Is this being considered for inclusion in the module?

alexmoreno’s picture

Added a new improved version against the beta. Will roll back for 1.0 now

alexmoreno’s picture

alexmoreno’s picture

One fix over the previous one (it didn't apply cleanly)

alexmoreno’s picture

And finally, this patch should apply cleanly to the 7.1 and the current 7.x version

alexmoreno’s picture

@broadway would be good if you could test it ?

Thanks

alexmoreno’s picture

FileSize
7.2 KB

one more, sorry. This one just following the naming conventions (as per our internal reviewing process)

mcrittenden’s picture

Status: Needs review » Needs work
+++ b/inc/amp.db.inc
@@ -0,0 +1,104 @@
+  // Simply is enabled?
+  $query = db_select('amp_node', 'n');
+  $query->condition('n.aid', $nodeid);
+  $query->addExpression('COUNT(*)');
+
+  $queryresults = $query->execute()->fetchField();
+
+  if ($queryresults > 0) {
+    // If the field already exists, just update it.
+    $aid = db_update('amp_node')
+      ->fields(array(
+        'status' => AMP_DISABLED,
+      ))
+      ->condition('aid', $nodeid, '=')
+      ->execute();
+  }
+  else {
+    // Otherwise, create a new one.
+    $aid = db_insert('amp_node')
+    ->fields(array(
+      'aid' => $nodeid,
+      'status' => AMP_DISABLED,
+    ))
+      ->execute();
+  }
+
+  return $aid;

I think all of this could be replaced with a db_merge() so you don't need a select query and you can remove the if/else.

Other than that, this looks really solid to me.

mcrittenden’s picture

Here's a re-roll of #10 that applies to 7.x-1.0.

mcrittenden’s picture

Version: 7.x-1.0-beta3 » 7.x-1.0
jpoirierpinto’s picture

alexmoreno’s picture

amp_db_enable_amp has disappeared but it's still called if I'm not wrong?

jpoirierpinto’s picture

FileSize
5.66 KB
mcrittenden’s picture

Here's a fixed version of #16 which was missing a file. This is basically #12 plus a fix for the fact that this patch wasn't playing nicely with the "POWER USER, Run entire page through filter" checkbox in the AMP settings.

alexmoreno’s picture

alexmoreno’s picture

FileSize
5.67 KB

Amends as per @mcrittenden review #11

alexmoreno’s picture

Tested together with #17. Looking good, moving to pending review

alexmoreno’s picture

Status: Needs work » Needs review
alexmoreno’s picture

FileSize
7.41 KB

pushing again the patch, something went wrong with the previous one

mcrittenden’s picture

Patch in #22 looks good except for 2 things:

1. It still tried to use the AMP view mode even when AMP was disabled on that node.
2. It still ran the DFP preprocessing stuff when AMP was disabled on that node.

Attached patch fixes both. See interdiff.txt.

I'm BRINGIN THAT GOOD GOOD

jbloomfield’s picture

Rerolled amp-disable-amp-node-specific-2802477-21-7.x.patch to fix a bug in the `amp_node_delete` function.

renzomb’s picture

Well this would be a little bit late but we develop a solution for this based on taxonomy terms.
We first use a logic to test amp in some nodes that have certain tag , and then we change for include amp in all nodes except that nodes that have one specific tag.

This is managed by the config in the aminitration for amp. If someone need it or is accepted behavior we can include this and make a patch or something.

mcrittenden’s picture

Here's an update to #24 with a one line change inside amp_node_view() which replaces this:

if (amp_node_is_enabled($build['#node']->nid)) {

With this:

if (amp_node_is_enabled($node->nid)) {

Because $build isn't available there. This seems to resolve an issue I was having where nodes with AMP disabled still displayed the amphtml
tag.

marcelovani’s picture

Patch works:
I applied the patch and run drush updb
The table was created and I can see the option on the node

But I found few issues and did some improvements, please review.

Interdiff for #26

+++ b/amp.module
@@ -6,6 +6,7 @@
 require_once dirname(__FILE__).'/amp.admin.inc';
+require_once dirname(__FILE__) . '/inc/amp.db.inc';

Since the include for amp.db.inc happens in 8 places, I made it a permanent include.
This gets cached by opcache and won’t make much difference for performance.

@@ -341,14 +342,17 @@ function amp_is_amp_request() {
+        // Node level check.
+        if (amp_node_is_enabled($node->nid)) {

Since amp_is_amp_request() is already checking if the content type is enabled, I think we could do the node level check on the same function. By doing this, we don’t need to check amp_node_is_enabled() every time we check amp_is_amp_request()

+++ b/amp.module
@@ -399,7 +392,7 @@ function amp_form_node_form_alter(&$form, &$form_state, $form_id) {
+      '#submit' => array('node_form_submit', 'amp_node_enabled_form_submit', 'amp_node_form_submit'),

@@ -407,42 +400,42 @@ function amp_form_node_form_alter(&$form, &$form_state, $form_id) {
+        '#submit' => array('node_form_submit', 'amp_node_enabled_form_submit', 'amp_node_form_submit_warnfix'),
...
+    $form['actions']['submit']['#submit'][] = 'amp_node_enabled_form_submit';

When I click ‘Save and view AMP page’ button it doesn’t call the form callback to enable/disable AMP on the node.

+++ b/amp.module
@@ -407,42 +400,42 @@ function amp_form_node_form_alter(&$form, &$form_state, $form_id) {
+    // Option to disable AMP.
+    // When adding new nodes, AMP will be turned on by default.
+    $amp_enabled = 1;
+    // If we are editing an existing node, get the value from database.
+    if (isset($form_state['node']->nid) && !amp_db_is_node_enabled($form_state['node']->nid)) {
+      $amp_enabled = 0;
+    }

When adding new nodes, nid won’t be available in the form amp_node_is_enabled($form_state['node']->nid)
Another thing, when editing nodes, it’s better to work with fresh data directly from DB

+++ b/amp.module
@@ -407,42 +400,42 @@ function amp_form_node_form_alter(&$form, &$form_state, $form_id) {
+function amp_node_enabled_form_submit(&$form, $form_state) {
+  if (isset($form_state['node']->nid)) {
+    $node_id = $form_state['node']->nid;
+    if ($form_state['values']['amp_enabled'] == 1) {
+      amp_db_enable_amp($node_id);
     }
-    catch (Exception $ex) {
-      watchdog('amp', "DB Exception: We couldn't disable the AMP version of the node");
+    else {
+      amp_db_disable_amp($node_id);
     }
+    amp_clear_cache($node_id);
   }

Why do we need to do a query to check if is is not enabled before making it enabled?
What is the reason for try{} ?

+++ b/amp.module
@@ -1519,31 +1508,30 @@ function amp_node_delete($node) {
+  cache_clear_all('amp:node_enabled:' . $id, 'cache');
...
+function amp_node_is_enabled($node_id, $cache_override = FALSE) {
   // Setup a cache ID
-  $cid = 'amp:node_enabled:' . $nodeid;
+  $cid = 'amp:node_enabled:' . $node_id;
 
   // If a cached entry exists, return it
-  if ($cached = cache_get($cid) && $cache_override != TRUE) {
-    $is_enabled = $cached->data;
+  if (($cache = cache_get($cid, 'cache')) && $cache_override != TRUE) {
+    $is_enabled = $cache->data;
   }
   else {
-    $is_enabled = amp_db_is_node_enabled($nodeid);
-
+    $is_enabled = amp_db_is_node_enabled($node_id);
     // Cache the result.
-    cache_set($cid, $is_enabled, 'cache_amp');
+    cache_set($cid, $is_enabled, 'cache');

It looks like we are getting data from the default cache in amp_node_is_enabled() cache_get($cid)
The cache_get above always returns True.
It turns out it is evaluating this as true:

if ($cache = cache_get($cid, 'cache') && $cache_override != TRUE) {

, fix: add extra parenthesis around the first part of the condition

Also, using a custom cache bin requires a schema definition. Without that, anyone testing this on Db cache will have errors because the table doesn’t get created. For simplicity I am changing the bin to the default value.

marcelovani’s picture

I forgot to include the inc folder on the previous patch

marcelovani’s picture

This is a better check to make sure we display the amp page correctly and display the normal page with

function amp_node_view($node, $view_mode, $langcode) {
- if (amp_is_amp_request()) {
- return;
+ if ($node = menu_get_object()) {
+ if (!in_array($node->type, amp_get_enabled_types()) || !amp_node_is_enabled($node->nid)) {
+ return;
+ }
}

// Show amphtml links on AMP-enabled nodes so search engines can find AMP.
- if ($view_mode == 'full' && node_is_page($node) && in_array($node->type, amp_get_enabled_types())) {
+ if ($view_mode == 'full' && node_is_page($node)) {
$uri = entity_uri('node', $node);
$uri['options']['query']['amp'] = NULL;
$uri['options']['absolute'] = TRUE;

marcelovani’s picture

Reading from google docs https://developers.google.com/amp/cache/update-ping

We can ping this url when the setting changes on the node.

Then change the logic that changes the access to AMP page when we change the flag. The only thing that would be needed is to remove the amphtml tag on the original node and reset the google cache as mentioned on google docs

marcelovani’s picture

Re-rolled patch

guardiola86’s picture

Status: Needs review » Needs work

The last submitted patch, 32: disable_amp_for-2802477-32.patch, failed testing.

guardiola86’s picture

New patch, I forgot to add the files in the inc folder.

guardiola86’s picture

Status: Needs work » Needs review
guardiola86’s picture

Wrong test. Uploaded again.

guardiola86’s picture

The last submitted patch, 24: amp-disable-amp-node-specific-2802477-24-7.x.patch, failed testing.

The last submitted patch, 26: amp-disable-amp-node-specific-2802477-26-7.x.patch, failed testing.

guardiola86’s picture

Patch and interdiff

The last submitted patch, 27: disable_amp_for-2802477-27.patch, failed testing.

The last submitted patch, 28: disable_amp_for-2802477-28.patch, failed testing.

The last submitted patch, 29: disable_amp_for-2802477-29.patch, failed testing.

The last submitted patch, 36: disable_amp_for-2802477-36.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: disable_amp_for-2802477-38.patch, failed testing.

guardiola86’s picture

guardiola86’s picture

guardiola86’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 47: disable_amp_for-2802477-46.patch, failed testing.

guardiola86’s picture

I'm having a strange issue. The test keeps on failing on this:
"[Warning] Line 9 of sites/all/modules/amp/amp.module:
require_once(/var/www/html/sites/all/modules/amp/inc/amp.db.inc): failed to open stream: No such file or directory
"

However, if I checkout the dev version of the module, download the patch, and apply it, the file amp.db.inc is created.

Can anyone test this? Thanks in advance.

marcelovani’s picture

Let me have a look into this.
Can you tell me about the interdiff.txt in #40, is that the interdiff between #40 and #31?

marcelovani’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

Re-rolled #31

marcelovani’s picture

We need some tests here

marcelovani’s picture

Status: Needs review » Needs work

The last submitted patch, 54: disable_amp_for-2802477-54.patch, failed testing.

marcelovani’s picture

Status: Needs work » Needs review
FileSize
11.76 KB

Fixing test pattern for query string

Status: Needs review » Needs work

The last submitted patch, 56: disable_amp_for-2802477-56.patch, failed testing.

marcelovani’s picture

Status: Needs work » Needs review
FileSize
11.91 KB

Can't figure out the output on drupal CI, it works locally, adding some debugging messages

Status: Needs review » Needs work

The last submitted patch, 58: debug.patch, failed testing.

marcelovani’s picture

Status: Needs work » Needs review
FileSize
11.75 KB

Little regex tweak

Status: Needs review » Needs work

The last submitted patch, 60: disable_amp_for-2802477-59.patch, failed testing.

marcelovani’s picture

Status: Needs work » Needs review
FileSize
11.62 KB
ajclamp’s picture

I've carried out an install of the patched module and was fully able to disable and enable AMP on individual pages without error. When disabled navigation to the amp version successfully displayed the Page not found message.

Re-enabling AMP on the node made the amp view active again.

Works as required to allow individual nodes to have AMP enabled or disabled.

Anonymous’s picture

Will a Dev or Stable/Security release be made that includes this feature?

alexmoreno’s picture

I'd suggest a stable, but Im not the maintainer on this project so is just that, a suggestion :-)

  • marcelovani committed 21d795c on 7.x-1.x
    Issue #2802477 by marcelovani, alexmoreno, guardiola86, mcrittenden,...
marcelovani’s picture

Status: Needs review » Fixed

We have been using this patch in production for a couple of months and it works fine.

Status: Fixed » Closed (fixed)

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

dv-vc’s picture

Has anyone worked out a D8 implementation of this functionality?

m4olivei’s picture

I'm also looking for a solution for D8.