AMP metadata for Top Stories support.

With this patch, ctools becomes a dependency, so we should explicitly declare that dependency and perhaps provide a warning to somebody who upgrades with this patch so they know to install ctools. I got an ugly error message when I went to a content type page to edit the metadata there and didn't have ctools installed.

When I enabled ctools and went to the content type, when I go to the AMP Metadata vertical tab, I only see a select element for Type, and the options are empty.

That's a content type with AMP enabled, so it seems like something is going awry.

Reroll.

To get this working, I needed to install Token module as well as File Entity module and Image URL Formatter module to get an image URL from an image field token.

I also needed to apply the patch in #2734535: Stop removing AMP metadata JSON script element to the theme.

So, this does work. Now that #2717641: Add support for 'Top Stories with AMP' is in, I want to double-check to see if any of the fixes we made in that issue need to apply here.

This won't work the same in D7 and D8, and that's fine, but we should try to find areas of consistency. The D7 version asks for a date changed and date modified token while the D8 version generates that automatically. The D8 version provides image styles to help with the organization logo and content image formatting. Do we want to do that here? Provide fallback for processing an img element output by the content image token? Those are the sort of things I plan to look at next.

Reviewed this, and I think there are few things from the D8 version that we'd want to get working with this as well.

1. Right now this only allows you to choose NewsArticle as the schema type. BlogPosting and Article should probably also be option.
2. In D8, we provide the option to select an image style for the org logo and the content image, and we provide default image styles for each.
3. We provide fallback processing for the content image token so that if the token outputs an img element, we parse that html to get the image URL. That would be nice, as it can be tricky to configure a token to output just the URL.
4. Right now we're only using one function from file entity module I think. If we absolutely need to depend on it, we should declare that as a dependency/requirement. If there's a way to write this code without that dependency, that could be nice, but if not, no problem.
5. Looks like token module definitely needs to be declared as a requirement. We could also add a token browser to the amp metadata tab to use a token for the organization name.

Probably wouldn't hurt to port over some of the help text that we have in the D8 module as well.

Good point about the date created and modified fields. We may want to open an issue to add those fields for D8 if that's a common pattern.

Spoke with Matt Robison about this earlier today. I'll take care of getting a few fixes done before we get this in.

This patch takes care of removing the file entity dependent functions as well adding token as a dependency.

I'll be working on adding additional schema types and fallback image processing before this goes in.

I'll open up a follow-up issue for image styles, as that can be done separately.

I'll also probably look at help text and printing the JSON in the same way we do D8 (avoiding slash escapes and using pretty print).

Took care of the remaining points from the review. Discussed this some more this afternoon with Matt R, and we decided we didn't need ctools exports, since each of the schema types have the same fields.

In a separate issue we can take care of image styles.

One last addition of a UI heading we had in the D8 version.

@mdrummund: We probably should add warnings on admin/config/content/amp/metadata about requiring token and ctools (like we have with token in the D8 module).

Unfortunately, this is not working for me, and none of the metadata are appearing. Here's another thing to fix. I'll keep digging to try and figure out what's going on.

+++ b/amp.module
@@ -511,6 +646,95 @@ function amp_node_view($node, $view_mode, $langcode) {
+        $image_info = getimagesize($image_url);

If there is no image, this will fail because getimagesize() requires a filename. Should check that $image_url exists before using it.

For what it's worth, this is what I'm seeing on my local:

This fixes the issues found in #13 and #14.

Also please note this requires running the dev version of amptheme, which prevents the json from being removed.

Looking better. Mostly nitpicks.

1. +++ b/amp.admin.inc
   @@ -270,3 +270,105 @@ function amp_get_formatted_status_list() {
   +    //'#required' => TRUE,
   

   Is there a reason not to make this required? If so, we should leave off this line. Otherwise, I don't see the harm in making it required.

2. +++ b/amp.install
   @@ -32,3 +48,149 @@ function amp_requirements($phase) {
   +function amp_schema() {
   ...
   +function amp_update_7101() {
   

   You could save a lot of code by putting the schema array in a separate helper function

3. +++ b/amp.install
   @@ -20,6 +20,22 @@ function amp_requirements($phase) {
   +      $requirements['amp_token'] = [
   +        'title' => t('Token module required for AMP'),
   +        'value' => t('Not installed'),
   +        'description' => t('The AMP module requires the <a href="@module">Token</a> module as a dependency. Please download and install Token to prevent errors with AMP.', ['@module' => 'https://www.drupal.org/project/token']),
   +        'severity' => REQUIREMENT_ERROR,
   +      ];
   +    }
   +    if (!module_exists('ctools')) {
   +      $requirements['amp_ctools'] = [
   +        'title' => t('Token module required for AMP'),
   +        'value' => t('Not installed'),
   +        'description' => t('The AMP module requires the <a href="@module">ctools</a> module as a dependency. Please download and install ctools to prevent errors with AMP.', ['@module' => 'https://www.drupal.org/project/ctools']),
   +        'severity' => REQUIREMENT_ERROR,
   +      ];
   

   It's nice that this appears on admin/reports/status, but I think it should appear on the metadata page, since it really only applies to existing AMP sites without ctools/token.

4. +++ b/amp.module
   @@ -424,6 +440,125 @@ function amp_node_form_submit_warnfix(&$form, $form_state) {
   +    $form['amp_metadata']['description'] = [
   +      '#type' => 'item',
   +      '#title' => 'Content information',
   +      '#description' => t('Provide information about your content for use in the Top Stories carousel in Google Search.'),
   +    ];
   

   This is Drupal 7, so this should be the old array() syntax rather than the short array syntax.

5. +++ b/amp.module
   @@ -424,6 +440,125 @@ function amp_node_form_submit_warnfix(&$form, $form_state) {
   +          $schema_type_options = [
   +            'Article' => 'Article',
   +            'NewsArticle' => 'NewsArticle',
   +            'BlogPosting' => 'BlogPosting',
   +          ];
   

   Should not use short array syntax

6. +++ b/amp.module
   @@ -511,6 +646,97 @@ function amp_node_view($node, $view_mode, $langcode) {
   +          $matches = [];
   

   Should not use short array syntax. I'll stop mentioning these and let you find the others. :)

7. +++ b/amp.module
   @@ -511,6 +646,97 @@ function amp_node_view($node, $view_mode, $langcode) {
   +        if ($image_url !== '') {
   

   This won't work. It needs to be something like if (!empty($image_url))

8. +++ b/amp.module
   @@ -511,6 +646,97 @@ function amp_node_view($node, $view_mode, $langcode) {
   +    // Add AMP metadata
   

   spacing needs to be fixed

9. +++ b/amp.module
   @@ -511,6 +646,97 @@ function amp_node_view($node, $view_mode, $langcode) {
   +      // Add global publisher info
   

   Missing period.

Realized that amp_schema and amp_update_7101 were holdovers from when we were using ctools exportables, so removed those. All items should be fixed.

1. +++ b/amp.admin.inc
   @@ -270,3 +282,118 @@ function amp_get_formatted_status_list() {
   +  $enabled_types = amp_get_enabled_types();
   +  if (isset($enabled_types)) {
   

   This if statement will always be true, so you will never get into the else statement. Did you mean if (!empty($enabled_types))?

2. +++ b/amp.admin.inc
   @@ -270,3 +282,118 @@ function amp_get_formatted_status_list() {
   +    if ($form_state['values']['amp_metadata_organization_logo'] != 0) {
   

   If org logo is a required field, could it ever equal 0?

3. +++ b/amp.module
   @@ -424,6 +440,125 @@ function amp_node_form_submit_warnfix(&$form, $form_state) {
   +        case 'image':
   +          $form_options['amp_metadata_config_' . $name]['#title'] = 'Article image for carousel';
   +          $form_options['amp_metadata_config_' . $name]['#description'] = t('
   +          <p>An article image to appear in the Top Stories carousel.</p>
   +          <p>Images must be at least 696px wide: refer to <a href="@image_guidelines">article image guidelines</a> for further details.</p>
   +          <p>Use tokens to provide the correct image for each article. Example token: [node:field_image]. The image field token likely varies by content type.',
   +            array('@image_guidelines' => 'https://developers.google.com/search/docs/data-types/articles#article_types')
   +          );
   +          $form_options['amp_metadata_config_' . $name]['#attributes'] = ['placeholder' => '[node:field_image]'];
   
   @@ -511,6 +646,97 @@ function amp_node_view($node, $view_mode, $langcode) {
   +        if (!empty($image_url)) {
   +          $image_info = getimagesize($image_url);
   

   Since "Article image for carousel" isn't actually image field you will need to either validate that a token was added when that field is saved, or check that $image_url is actually a valid image before using it.

More fixes based on reviews. Thanks for the suggestions!

The changes to the image logic make it so content type overrides to image metadata no longer appear. (I did not troubleshoot.)

image_get_info requires a stream-wrapped URI. This now converts an absolute URL to a stream-wrapped URI, so this is working again.

I'd say this is ready to go. Nice work, guys! Committed to 7.x-1.x.