Convert _amp_get_amp_url(), _amp_build_metadata(), _amp_get_merged_metadata(), and _amp_prepare_metadata_json() in amp.module into services.

amp_build_metadata() can probably be a render element, since it is outputting a render array. Generic service classes for _amp_get_merged_metadata and _amp_prepare_metadata_json.

Doing this will make this code more extensible and testable.

CommentFileSizeAuthor
#7 interdiff-5-7.txt3.05 KBRainbowArray
#7 2745221-7-refactor-amp-metadata.patch36.7 KBRainbowArray
#5 interdiff-2745221-4-5.txt4.06 KBRainbowArray
#5 2745221-5-refactor-amp-metadata.patch36.81 KBRainbowArray
#4 interdiff-2745221-3-4.txt15.46 KBRainbowArray
#4 2745221-4-refactor-amp-metadata.patch35.87 KBRainbowArray
#3 2745221-refactor-amp-metadata.patch24.05 KBRainbowArray
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdrummond created an issue. See original summary.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Assigned: Unassigned » RainbowArray
RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Active » Needs work
FileSize
2745221-refactor-amp-metadata.patch24.05 KB

Refactored the first chunk of the amp.module code. This takes care of the refactoring of the JSON metadata preparation. Tried to construct this in a way so that in theory if somebody wanted to extend this to add additional metadata JSON, it should be possible to do so.

More refactoring necessary, but this was one of the biggest chunks.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
FileSize
2745221-4-refactor-amp-metadata.patch35.87 KB
interdiff-2745221-3-4.txt15.46 KB

This refactors the remainder of the helper functions in amp.module.

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Needs work » Needs review
FileSize
2745221-5-refactor-amp-metadata.patch36.81 KB
interdiff-2745221-4-5.txt4.06 KB

This adds an interface for AmpPrepareMetadataJson. That seems like the class that would most likely be reused to do some different things with the metadata.

With that, I think this is ready for review.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Needs review » Needs work

Looking good. Working well. Here's some initial feedback:

  1. +++ b/src/Utility/AmpMergeMetadata.php
@@ -0,0 +1,123 @@
+   * @var array $metadataEntities;

    Just "@var array" should be enough 

  2. +++ b/src/Utility/AmpMergeMetadata.php
@@ -0,0 +1,123 @@
+   * @var array $ampGlobalMetadata;

    Just "@var array" should be enough 

  3. +++ b/src/Utility/AmpMergeMetadata.php
@@ -0,0 +1,123 @@
+   * @var array $ampTypeMetadata;

    Just "@var array" should be enough (and $ampTypeMetadata seems incorrect) 

  4. +++ b/src/Utility/AmpMergeMetadata.php
@@ -0,0 +1,123 @@
+   * @var \Drupal\Core\Entity\EntityTypeManager $entityTypeManager

    I think you mean @var \Drupal\Core\Entity\EntityTypeManagerInterface 

  5. +++ b/src/Utility/AmpMergeMetadata.php
@@ -0,0 +1,123 @@
+use Drupal\Core\Entity\EntityTypeManager;

    I think this should be use Drupal\Core\Entity\EntityTypeManagerInterface; 

  6. +++ b/src/Utility/AmpMergeMetadata.php
@@ -0,0 +1,123 @@
+  public function __construct(EntityTypeManager $entity_type_manager) {

    This should be $EntityTypeManagerInterface 

  7. +++ b/src/Utility/AmpPrepareMetadataJson.php
@@ -0,0 +1,414 @@
+  public function getJson(array $amp_metadata_settings, $canonical_url, NodeInterface $node) {
+    $this->ampMetadataSettings = $amp_metadata_settings;
+    $this->canonicalUrl = $canonical_url;
+    $this->node = $node;
+    if (!empty($this->ampMetadataSettings) && !empty($this->canonicalUrl) && !empty($this->node)) {

    This does not look right. You should check before you assign the variables to properties, not after. Another way to do this would be to assign default values for method parameters.

And something in this patch is causing this error, but I'm not sure what:

Warning: htmlspecialchars() expects parameter 1 to be string, object given in Drupal\Component\Utility\Html::escape() (line 402 of /var/www/amp/docroot/core/lib/Drupal/Component/Utility/Html.php).

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
FileSize
2745221-7-refactor-amp-metadata.patch36.7 KB
interdiff-5-7.txt3.05 KB

This should address the feedback from points 1-7 in #6. I didn't encounter the htmlspecialchars() error. Hopefully the proper checking of the variables in #7 is preventing that? If that error still comes up, maybe a screenshot would help? That line that is referenced is as follows if that's helpful:

    return htmlspecialchars($text, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');

Thanks so much for the feedback mtift!

RainbowArray’s picture

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

  • mtift committed 59091e3 on 8.x-1.x authored by mdrummond 
    Issue #2745221 by mdrummond, mtift: Refactor AMP metadata render array...
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Needs review » Fixed

Nice work, @mdrummund. Committed to 8.x-1.x.

Status: Fixed » Closed (fixed)

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

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift at Lullabot commented