Serialization is meant to take a data structure and convert it to a serialized string format.

CommentFileSizeAuthor
#86 metatag-2636852-86.patch692 bytesShawn DeArmond
#80 make_metatag_fields-2636852-80.patch1.94 KBadamzimmermann
#70 metatag-n2636852-70.interdiff.txt835 bytesDamienMcKenna
#69 metatag-n2636852-69.interdiff.txt5.82 KBDamienMcKenna
#69 metatag-n2636852-69.patch12.86 KBDamienMcKenna
#68 interdiff-2636852-67-68.txt4.48 KBhanoii
#68 metatag-n2636852-68.patch7.04 KBhanoii
#67 metatag-n2636852-67.patch6.66 KBhanoii
#64 metatag-n2636852-64.patch5.18 KBhanoii
#64 interdiff-2636852-62-64.txt523 byteshanoii
#62 metatag-n2636852-62.patch5.18 KBhanoii
#62 interdiff-2636852-60-62.txt5.06 KBhanoii
#60 interdiff-2636852-54-60.txt672 byteshanoii
#60 metatag-n2636852-60.patch3.82 KBhanoii
#56 interdiff-2636852-54-56.txt910 bytesskorzh
#56 metatag-n2636852-56.patch3.72 KBskorzh
#54 metatag-n2636852-54.patch3.69 KBDamienMcKenna
#44 metatag-n2636852-44.patch742 bytesfjgarlin
#37 metatag-n2636852-37.patch4.8 KBDamienMcKenna
#32 interdiff-2636852-32.txt922 bytesGrayside
#32 support-serialization-api-json-2636852-32.patch4.18 KBGrayside
#31 interdiff-2636852-31.txt1.66 KBGrayside
#31 support-serialization-api-json-2636852-31.patch4.13 KBGrayside
#30 interdiff.2636852-30.txt4.85 KBGrayside
#30 support-serialization-api-json-2636852-30.patch4 KBGrayside
#29 support-serialization-api-json-2636852-29.patch2.5 KBGrayside
#21 patch-2636852-split_metatag_on_json.patch2.7 KBpcho
#19 metatag-n2636852-19.patch2.38 KBDamienMcKenna
#18 patch-2636852-split_metatag_on_json.patch2.41 KBpcho
#14 metatag-n2636852-14.patch1.98 KBDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

vincic created an issue. See original summary.

Anonymous’s picture

Anonymous’s picture

When rendering a Node in Drupal the hreflang links are automatically added to the page. When using JSON you don't get that information.

Since it is part of the metadata I think it should be in Metatag and an working implementation is in this branch https://github.com/vincic/metatag/tree/hreflang and the diff https://github.com/vincic/metatag/commit/a7925734d3157fe0f7eae4c159f5b33...

What do you think?

DamienMcKenna’s picture

The hreflang functionality should be handled similarly to what's in the D7 branch, and at the very least should be in a separate issue.

Anonymous’s picture

DamienMcKenna’s picture

Priority: Major » Normal
Status: Active » Needs work

FYI I posted a comment to the PR indicating that some of the lines needed to have their indentation fixed.

Anonymous’s picture

Ok will fix the indentation. One thing I noticed now with latest metatag trunk and Drupal 8.0.2 is that the MetatagField is not generated in JSON unless I go and resave a node. That is for all existing nodes I have.

I see that there are some update steps that delete field data if it is the same as default, is that the reason the serialization to JSON of the field is not triggered?

DamienMcKenna’s picture

Issue tags: +Needs tests

I guess this needs tests then?

Anonymous’s picture

Maybe it needs tests but how do I simulate an existing node in test?
That is nodes that were created before this version of metatag? The problem doesn't exist on new nodes or existing if I resave them.

DamienMcKenna’s picture

It sounds like this issue needs to be split off from the main JSON feature request.

So the tests for this issue should load the JSON path for a node it creates and confirm that the data is valid JSON.

larowlan’s picture

Will this allow POST? If so, please ping me Damien, as there are security considerations without #2589203: Explore moving to one field item per metatag instead of a serailized blob

DamienMcKenna’s picture

I'm not currently intended to allow POSTing.

frob’s picture

Is there a patch available for this functionality? I need this for a site I am building. How would I go about adding this to a make file with no patch? I could fix the indentation issues but I cannot edit someone else's PR.

Should I pull down the PR commit and base a patch off of it and submit it? I am not clear on the workflow here.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
1.98 KB

In patch format.

DamienMcKenna’s picture

Status: Needs review » Needs work

Changing it back to "Needs work" as it needs some form of test to ensure that it's working correctly.

larowlan’s picture

  1. +++ b/metatag.services.yml
    @@ -14,3 +14,8 @@ services:
    +  metatag.normalizer.metatagfielditem:
    +    class: Drupal\metatag\Normalizer\FieldItemNormalizer
    +    tags:
    +      - { name: normalizer, priority: 30 }
    
    +++ b/src/Normalizer/FieldItemNormalizer.php
    @@ -0,0 +1,47 @@
    +class FieldItemNormalizer extends NormalizerBase {
    

    NormalizerBase isn't defined in core, its only in serialization module. So you cannot universally declare the service unless you add a dependency to the serialization module which provides the base class. Instead you need to add a ServiceProviderInterface, see this example in file_entity for how to do it.

  2. +++ b/src/Normalizer/FieldItemNormalizer.php
    @@ -0,0 +1,47 @@
    +  /**
    +   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
    +   */
    

    Should just be {@inheritdoc}

  3. +++ b/src/Normalizer/FieldItemNormalizer.php
    @@ -0,0 +1,47 @@
    +    if ($format == 'json' || $format == 'hal_json') {
    

    nit: hal_json isn't available unless hal module is enabled.
    nit: what about csv_serialization and XML?

pcho’s picture

Hi all,

I'm encountering this issue on a current project, and I took vincic's PR in #2 and added my own alterations to the normalizer (I also updated the branch with the latest changes from damienmckenna's repo). Essentially, I tried to maintain the same format (for backwards compatibility) and included an additional array "values" for easier reference. Here's a screenshot of what it looks like:

http://content.screencast.com/users/pchop2/folders/Jing/media/23452d3c-c...

Let me know if this fits the profile or if this is the direction we want to take with this. I do feel this feature is very important so we're not all writing our own abstractions to extract the values from the single string blob.

https://github.com/damienmckenna/metatag/pull/4

pcho’s picture

Patch for convenience.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

Minor tweaks.

DamienMcKenna’s picture

Still need some tests to confirm it works as intended.

pcho’s picture

Status: Needs review » Needs work
FileSize
2.7 KB

Sorry, one more fix to the PR. Noticed none of the tokens were parsing correctly, so I added that commit. New patch attached for convenience.

pcho’s picture

Status: Needs work » Needs review
Grayside’s picture

+++ b/src/Normalizer/FieldItemNormalizer.php
@@ -0,0 +1,66 @@
+    if (strpos($format, 'json') !== false) {

Nothing about this seems specific to flavors of JSON.

+++ b/src/Normalizer/FieldItemNormalizer.php
@@ -0,0 +1,66 @@
+          // Kept for backwards compatibility.
+          'value' => $values['value'],

B/C for PHP serialized data doesn't seem useful to me. It's really more of a bug to be factored out than something to be preserved.

DamienMcKenna’s picture

One minor thing - 'false' should be in uppercase, per the Drupal coding standards.

pcho’s picture

@Grayside

1. I'm open to suggestions here. Though the reasoning behind this is that you can indeed define your own machine label to the format being passed through this normalizer. The original patch matched two cases, but I wanted this to be a little more flexible, hence why I decided to do it based on matches of the keyword "json".

2. I feel this is useless, too. Though unless we plan on yielding a note in the CHANGELOG saying this is going away, I prefer not to disrupt anyone who might be using this value...just in case. Probably a better argument since this module is still in beta state. We in agreement @DamienMckenna?

(Commit to fix coding styles in PR, but I didn't create a new patch...yet)

Grayside’s picture

Implementing the supportsNormalization() and supportsDenormalization() methods is how to determine if the normalizer should fire at all.

I don't see anything in this class that is specific to JSON, the other encoders can take the resulting array structure just as well. So I'm advocating for simply removing that conditional check and broadening this issue from supporting normalization as JSON to "Support metatag normalization via the Serialization API."

Grayside’s picture

Gets some of this in watchdog for me:

Notice: Undefined index: lang in Drupal\metatag\Normalizer\FieldItemNormalizer->normalize() (line 61 of /var/www/build/html/modules/contrib/metatag/src/Normalizer/FieldItemNormalizer.php).

DamienMcKenna’s picture

Status: Needs review » Postponed
Parent issue: » #2563615: Plan for Metatag 8.x-1.1 release

This is excellent, thank you so much for working on it!

In the interest of getting out the door sooner, I'm punting this until after 1.0 is released.

Grayside’s picture

Here's a straight reroll of #21, more changes inbound tomorrow.

Grayside’s picture

Okay, incrementing forward. Summary of the changes:

  • Removed JSON restriction. This is just as viable for any other encoding format as other fields currently handled generically by the Serialization module.
  • Use unserialized tags as value, dropping values. PHP serialized data is not useful.
  • Move unserialization to helper method. From a separation of concerns standpoint, I think that should be handled in MetatagManager or the like but I'm keeping the relevant code contained here.
  • Switch token replacement to use the MetatagToken service, with a tweak to that service to allow us to explicitly disable the clear setting.
  • Some light cleanup on coding standards and normalizer conventions.

Next Steps

  • Add supportsDenormalization method returning FALSE.
  • Consider if token replacement is worthwhile at all given we do not have access to entity to which the metatag field is attached. Arguably the full entity should be stashed in the context for these kinds of issues.

Last thought, #2589203: Explore moving to one field item per metatag instead of a serailized blob would probably obviate the need for this patch at all. Following the full expectations of the entity system in D8 is really helpful, especially for these more data structure oriented features.

Grayside’s picture

Right, FieldItemBase::getEntity()! Also added supportsDenormalziation().

Grayside’s picture

Missed the secondary wrapping of the normalized values in an array. This allows multiple values for the field to be merged together, and facilitates the field being represented in the JSON with a top-level array like all the other fields.

petiar’s picture

Hello everyone, maybe I do not understand this well enough, but even though I applied this patch successfully, I can't see any metatags in my JSON output. Is there anything else I need to do? Thanks a lot.

Grayside’s picture

Petiar: did you apply the patch then clear the cache?

DamienMcKenna’s picture

Status: Postponed » Needs review

Lets have the patches reviewed anyway.

DamienMcKenna’s picture

Status: Needs review » Postponed
DamienMcKenna’s picture

Status: Needs review » Needs work

This really needs tests, loading a node via JSONAPI results in the following error:

( ! ) Fatal error: Call to a member function getInclude() on array in modules/contrib/jsonapi/src/Normalizer/Value/FieldNormalizerValue.php on line 55

DamienMcKenna’s picture

The 1.0 release is being bumped up, so the everything else is being put on the back burner for now.

DamienMcKenna’s picture

Did some of the internal APIs in JSONAPI change during 2016? Any idea why this would be throwing the error mentioned in #38?

Grayside’s picture

I never tested this against JSONAPI. Offhand, maybe comparative normalizer service priority is an issue?

I'm using this successfully with JSON, HAL JSON, and a customized variant of HAL JSON.

DamienMcKenna’s picture

Oh, in that case we definitely need some tests to cover the functionality.

Can you please start by providing some steps for seeing the JSON output?

Grayside’s picture

@DamienMcKenna I do not have time to create tests right now, but happy to help with some descriptions.

To see the output of this, you will need:

  • Enable REST module
  • Enable Metatag module with this patch
  • Enable the REST UI module
  • Get a Metatags field onto an entity type, let's say node.
  • Using the admin provided by REST UI module, enable Node GET resource. Enable JSON as the format and Cookie as the auth type.
  • Create a new node with customized metatags.
  • Navigate to /node/nid?_format=json
  • Look for your metatag field in the output. A browser plugin will help view the JSON.

Here is a slice of my metatag JSON Schema definition for my project's metatag field. (Built with Schemata module and #2825218: Support Metatag module

In practice, it will look something like this:

field_meta_tags: [
    {
        value: {
             abstract: "My custom abstract!"
        },
        lang: "en"
    }
],
fjgarlin’s picture

FileSize
742 bytes

When using REST services (in core), if you populate the metatags with specific values these will get served by Drupal. However, if you leave the default ones, Drupal won't serve them.

I think this is related to this thread but I'm not totally sure that it completely solves the problem though. This patch will make the metatag module recognize a "rest" route and therefore compute the metatags correctly.

fjgarlin’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

This really needs tests to work out what the output should be and then work backwards.

Viewing the JSON output for a node should show the same meta tags that output when viewing a node's HTML output.

Grayside’s picture

Issue summary: View changes

@fjgarlin I think that patch is unrelated. Certainly that patch does not do any of the necessary existing implementation from #37.

@DamienMcKenna serialization converts a data structure to a string representation. I agree, it is a weakness in the current implementation that the default values are not folded in. A large part of the problem is that Metatag uses it's own bespoke layer for data storage that is not well-aligned with the Entity system. I understand where the reasons for that made strong sense in D6 and D7, but I think the cost of it in D8 may be too high. Things like #2589203: Explore moving to one field item per metatag instead of a serailized blob would go a long way to helping better align Metatag with core and other modules, as it would be a stepping stone to building defaults, token processing, and entity API interactions into the field system which can be used much more naturally in serialization, multilingual, migration, and so on.

I do not think this issue should try for perfect. As it is now, this patch enables a Drupal to Drupal content deployment use case, and would also facilitate modules such as default_content, both cases of which you wouldn't want the default values.

fjgarlin’s picture

fjgarlin’s picture

@Grayside I've created a new issue with this. Find it here: https://www.drupal.org/node/2857544

DamienMcKenna’s picture

DamienMcKenna’s picture

Status: Needs review » Needs work

Back to needing tests.

DamienMcKenna’s picture

Both #2857544 and #2853515 have been committed. I've committed the tiny improvement to MetatagToken::replace() that's general enough to help module-wide, so this can then focus on the JSON integration.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
3.69 KB

The rest of the patch.

Status: Needs review » Needs work

The last submitted patch, 54: metatag-n2636852-54.patch, failed testing.

skorzh’s picture

Hi guys, we use jsonapi module on our site, and #54 patch returns 'Call to a member function getInclude() on array’ error.

Not sure how to fix it perfectly, just attached a patch that works with jsonapi module.

Sebastien M.’s picture

Status: Needs work » Needs review
Sebastien M.’s picture

Tests are only run under 'Needs review' status.

hanoii’s picture

Just tried #54 and worked OK. I don't think we should have a dependency on jsonapi whatever the case.

hanoii’s picture

The nature of the json data should also include default tags with its token replaced.

Attached is a simple addition to #54 that adds this.

As for #56, I am hiding it because it's confusing for the health of this issue, eventually we can look into sort this better. It might be more a bug on jsonapi than this issue.

hanoii’s picture

Status: Needs review » Needs work

Hmm, the defaults doesn't work if the metatag field is empty (the normalizer is not run).

Somehow I believe this should be part of this patch too.

hanoii’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
5.18 KB

I reworked the patch quite a bit. Here are my modifications

- Instead of normalizing the fielditem, I am now adding a wrapper MetatagFieldItemList so that we can target the field on its own and not the field item. This allows me to normalize even an empty list and add the default metatags.
- Adapt the original field item normalization code to the list container, which is just de-reference it one more level and renamed the normalization class.
- Have a different output for hal_json than for json to match other fields' format.
- Renamed the service definition to make it more similar to core (serialization.services.yml or hal.services.yml)

This seems to be working for both json and json+hal and its adding the defaults even if the field is empty (no metatag is overridden).

hanoii’s picture

[just adding proper organization contribution]

hanoii’s picture

hanoii’s picture

Status: Needs review » Needs work

I just realized that an empty metatags field was only happening for the nodes that already existed after the metatag was enabled. For those the field was empty so no metatag was output on the normalizer. Once you create a new node or edit an existent one, the field has data and the old patch also probably work.

Hmmm, I still kind of like what I did, it will help sites with existing content.

On the other hand, if the content has no field, there's no metatags on the json, so the defaults are still not there.

I think this still needs work. Maybe do the normalizer on the entity level, both?

hanoii’s picture

Do something for Drupal\serialization\Normalizer\ContentEntityNormalizer too and add defaults there, but also on the field and leave the client of the normalized data decide what to do if both are present?

Actually I just looked and I think an entity normalizer that just calls:

metatag_get_tags_from_route()

is the way to go, it will actually simplify the code quite a bit and make the MetatagListWrapper class unnecessary.

Will try to make this on another patch soon.

hanoii’s picture

Well, I gave this another shot. I am not providing an interdiff here as I kind of changed the approach.

I am overall happy with the patch except for the note I am making on the field that the normalization is not happening there.

Thing is that an entity can have a meta tags field or not, but the entity could have metatags as well.

So this patch provides:

- A virtual computed entity field on hook_entity_base_field_info_alter(). with its own dummy class.
- Two normalizers (json and hal) for that dummy class that takes a similar approach to metatag_get_tags_from_route(). This replaces all tokens, uses the field is present and does what it normally does on the page.
- Kept the actual fielditemnormalizer for when the meta tag is present, but I am just showing a string saying: 'Metatags are normalized in the metatag field.'.

The last item is not super bad, but its odd. But the whole normalizer logic is kind of odd right now on drupal.

hanoii’s picture

Status: Needs work » Needs review
FileSize
7.04 KB
4.48 KB

Simplified the code considerable by reusing the main metatag functions with a small change some function declaration.

DamienMcKenna’s picture

DamienMcKenna’s picture

Minor tweaks (not bothering with tests).

DamienMcKenna’s picture

FYI the testbot is kinda stuck right now (https://www.drupal.org/pift-ci-job/728314), but the branch tests failing are a symptom of a problem with 8.4 rather than 8.3.

  • DamienMcKenna committed e30a38d on 8.x-1.x authored by vincic
    Issue #2636852 by hanoii, DamienMcKenna, Grayside, pcho, skorzh,...
DamienMcKenna’s picture

Status: Needs review » Fixed
Parent issue: » #2563617: Plan for Metatag 8.x-1.2 release

I've done a bunch of local testing. This is in reasonable state for an initial release, so thank you everyone! Committed!

JonathanFontesCaetsu’s picture

Hello,

I've recently update my metatag module for the most recent version

- Updating drupal/metatag (1.1.0 => 1.2.0): Downloading (100%) 

So far, so good. But when i was installing "Webprofile" give-me this error:

Fatal error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in /opt/application/web/modules/contrib/metatag/src/Normalizer/FieldItemNormalizer.php on line 10

I think is related to this issue. Anyone know some tips to resolve my issue?

Best Regards,
Jonathan Fontes

hanoii’s picture

@JonathanFontesCaetsu that's odd.

(1) what's "webprofile" that you were installing?

(2) have you tried clearing cache? drush cr

JonathanFontesCaetsu’s picture

@hanoii the webprofile which i'm refering is come with Devel Module.

DamienMcKenna’s picture

Please open a new issue to discuss this. Thanks.

DamienMcKenna’s picture

adamzimmermann’s picture

I realize that jsonapi was addressed earlier in this thread and it was mentioned that it should be a problem for the jsonapi module. I'm happy to move this discussion if that is still the case. However, I see we are supporting HAL in Normalizer\MetatagHalNormalizer, so perhaps we support other formats as well? Adding code that uses metatag modules functions/classes to obtain values and provide custom rendering of them seems like a concern of this module to me (with my limited exposure to this issue).

Anyways, I found this issue after trying to use the jsonapi module and this module together, and running into the following error which renders the API unusable.

Fatal error: Call to a member function setPropertyType() on array in [....]/web/modules/contrib/jsonapi/src/Normalizer/EntityNormalizer.php on line 224

The issue is that the Normalizer\MetatagNormalizer::normalize method simply returns an array, while the normalization class (Normalizer\EntityNormalizer) expects an \Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue object as seen on line 220.

I'm new to normalization in D8, so I didn't want to alter MetatagNormalizer. So instead I created a new class for normalizing the data for the jsonapi module.

namespace Drupal\metatag\Normalizer;

use Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue;
use Drupal\jsonapi\Normalizer\Value\FieldItemNormalizerValue;

/**
 * Converts the Metatag field item object structure to METATAG array structure.
 */
class MetatagApiJsonNormalizer extends MetatagNormalizer {

  /**
   * The formats that the Normalizer can handle.
   *
   * @var array
   */
  protected $format = ['api_json'];

  /**
   * {@inheritdoc}}
   */
  public function normalize($field_item, $format = NULL, array $context = []) {
    $data = parent::normalize($field_item, $format, $context);
    $normalized = new FieldNormalizerValue(
      [new FieldItemNormalizerValue($data)],
      1
    );
    return $normalized;
  }
}

This and a quick metatags.service.yml entry got me almost there and solved the initial error.

  metatag.normalizer.metatag.jsonapi:
    class: Drupal\metatag\Normalizer\MetatagApiJsonNormalizer
    tags:
      - { name: normalizer, priority: 32 }

However, then I ran into a new issue.

Fatal error: Call to undefined method Drupal\Core\StringTranslation\TranslatableMarkup::getInclude() in [...]/web/modules/contrib/jsonapi/src/Normalizer/Value/FieldNormalizerValue.php on line 57

I tracked this down to the fact that Drupal\metatag\Normalizer\FieldItemNormalizer has this.

return t('Metatags are normalized in the metatag field.');

If I comment out the metatag.services.yml entry for that file, then the API works great and I can see metatag module information in the returned JSON response and I'm a happy camper. However, of course I have now hacked the module up a bit.

So two questions.

  1. If I re-roll this patch with jsonapi support as outlined above, are we interested in that? If not, how should we handle this?
  2. What does the Drupal\metatag\Normalizer\FieldItemNormalizer class do for us, and can we remove it or fix what it is returning so it doesn't break things? I only tested things by loading up the jsonapi endpoints.

Hope this all helps. I may submit a patch shortly simply to help the discussion, and so I can put a link in my composer file to share this with the rest of my team. This seems highly related to this issue, but I'm also happy to file a new issue.

adamzimmermann’s picture

hanoii’s picture

So, my two cents here as I worked mostly on the patch that got committed.

The reason I added both json and json+hal is because both are supported in core, so it made sense that a contrib support at least those two methods.

jsonapi is a different format/module/contrib, and it overrides a lot of normalizers to take care of what it needs (I am not familiar with jsonapi).

So I think this should really be added to the jsonapi module, which should take care of normalizing its own known entities.

Hopefully your patch is not too different and you can your work to a patch for json api. It's a similar thing only adding the service to that and making sure the priority is on top.

As for the string, well it's an odd addition, but as I explained before, normalizer are odd. The only benefit of that legend is that one adding a fields and using the core normalizers could at least realizes that the metatagfield is there only that the metatags should be on a different property. That too could be overridden on a new normalizer on jsonapi.

hanoii’s picture

A quick review on your patch and it should be almost exactly the same, with one extra use for the metatag normalizer and a different namespace.

DamienMcKenna’s picture

Could the jSONAPI support please be moved into a separate issue? Thanks.

adamzimmermann’s picture

#2901253: Add normalization support for metatag module filed in the jsonapi issue queue. I will create a new issue here if they say this is where it should live.

Thanks for the quick replies and input here.

Status: Fixed » Closed (fixed)

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

Shawn DeArmond’s picture

FileSize
692 bytes

Just in case anyone else is interested in @adamzimmermann's hack, here's a patch file. This is NOT a full solution, but it'll keep it running until the real solution is presented.

MasteRRR’s picture

The solution doesn't work for me:
The metatag field is still empty. Does anybody know when this bag will be fixed?

Drupal 8.5.3 + metatag 8.x-1.5. + jsonapi 8.x-1.16.

DamienMcKenna’s picture

@MasteRRR: This makes the Metatag field's data available as JSON. It does not make the output meta tags available in JSON format. What I think you'll want is #2971192: Provide full JSON output of all rendered meta tags.