Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#62 | disable_amp_for-2802477-61.patch | 11.62 KB | marcelovani |
| |||
#60 | disable_amp_for-2802477-59.patch | 11.75 KB | marcelovani |
#58 | debug.patch | 11.91 KB | marcelovani |
#56 | disable_amp_for-2802477-56.patch | 11.76 KB | marcelovani |
#54 | disable_amp_for-2802477-54.patch | 11.75 KB | marcelovani |
Comments
Comment #2
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedComment #3
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedComment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThis is a promised feature mentioned in the Readme.md file in the AMP download.
Is this being considered for inclusion in the module?
Comment #5
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedAdded a new improved version against the beta. Will roll back for 1.0 now
Comment #6
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedComment #7
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedOne fix over the previous one (it didn't apply cleanly)
Comment #8
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedAnd finally, this patch should apply cleanly to the 7.1 and the current 7.x version
Comment #9
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commented@broadway would be good if you could test it ?
Thanks
Comment #10
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedone more, sorry. This one just following the naming conventions (as per our internal reviewing process)
Comment #11
mcrittenden CreditAttribution: mcrittenden commentedI 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.
Comment #12
mcrittenden CreditAttribution: mcrittenden commentedHere's a re-roll of #10 that applies to 7.x-1.0.
Comment #13
mcrittenden CreditAttribution: mcrittenden commentedComment #14
jpoirierpinto CreditAttribution: jpoirierpinto commentedComment #15
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedamp_db_enable_amp has disappeared but it's still called if I'm not wrong?
Comment #16
jpoirierpinto CreditAttribution: jpoirierpinto commentedComment #17
mcrittenden CreditAttribution: mcrittenden commentedHere'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.
Comment #18
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedComment #19
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedAmends as per @mcrittenden review #11
Comment #20
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedTested together with #17. Looking good, moving to pending review
Comment #21
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedComment #22
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedpushing again the patch, something went wrong with the previous one
Comment #23
mcrittenden CreditAttribution: mcrittenden commentedPatch 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
Comment #24
jbloomfield CreditAttribution: jbloomfield at BBC Worldwide commentedRerolled amp-disable-amp-node-specific-2802477-21-7.x.patch to fix a bug in the `amp_node_delete` function.
Comment #25
renzomb CreditAttribution: renzomb commentedWell 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.
Comment #26
mcrittenden CreditAttribution: mcrittenden commentedHere's an update to #24 with a one line change inside
amp_node_view()
which replaces this:With this:
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.
Comment #27
marcelovaniPatch 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
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.
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()
When I click ‘Save and view AMP page’ button it doesn’t call the form callback to enable/disable AMP on the node.
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
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{} ?
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:
, 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.
Comment #28
marcelovaniI forgot to include the inc folder on the previous patch
Comment #29
marcelovaniThis 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;
Comment #30
marcelovaniReading 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
Comment #31
marcelovaniRe-rolled patch
Comment #32
guardiola86 CreditAttribution: guardiola86 commentedUpdated patch
Comment #34
guardiola86 CreditAttribution: guardiola86 commentedNew patch, I forgot to add the files in the inc folder.
Comment #35
guardiola86 CreditAttribution: guardiola86 commentedComment #36
guardiola86 CreditAttribution: guardiola86 commentedWrong test. Uploaded again.
Comment #37
guardiola86 CreditAttribution: guardiola86 commentedComment #40
guardiola86 CreditAttribution: guardiola86 commentedPatch and interdiff
Comment #46
guardiola86 CreditAttribution: guardiola86 commentedComment #47
guardiola86 CreditAttribution: guardiola86 commentedComment #48
guardiola86 CreditAttribution: guardiola86 commentedComment #50
guardiola86 CreditAttribution: guardiola86 commentedI'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.
Comment #51
marcelovaniLet me have a look into this.
Can you tell me about the interdiff.txt in #40, is that the interdiff between #40 and #31?
Comment #52
marcelovaniRe-rolled #31
Comment #53
marcelovaniWe need some tests here
Comment #54
marcelovaniAdded a test
Comment #56
marcelovaniFixing test pattern for query string
Comment #58
marcelovaniCan't figure out the output on drupal CI, it works locally, adding some debugging messages
Comment #60
marcelovaniLittle regex tweak
Comment #62
marcelovaniComment #63
ajclamp CreditAttribution: ajclamp as a volunteer commentedI'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.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedWill a Dev or Stable/Security release be made that includes this feature?
Comment #65
alexmoreno CreditAttribution: alexmoreno commentedI'd suggest a stable, but Im not the maintainer on this project so is just that, a suggestion :-)
Comment #67
marcelovaniWe have been using this patch in production for a couple of months and it works fine.
Comment #69
dv-vc CreditAttribution: dv-vc commentedHas anyone worked out a D8 implementation of this functionality?
Comment #70
m4oliveiI'm also looking for a solution for D8.