Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper created an issue. See original summary.

Grimreaper’s picture

Parent issue: » #2909022: Write tests
gidel’s picture

Assigned: Unassigned » gidel
gidel’s picture

Assigned: gidel » Unassigned
Status: Active » Needs review
Issue tags: +Smile Contribution Tour 2019
FileSize
7.76 KB

Hello,

here the patch to review.

Status: Needs review » Needs work
Grimreaper’s picture

Hello,

Thanks for the patch.

Unfortunately it highlights that there is a problem with metatag fields.

quicksketch’s picture

Title: Add a metatag field in node type used for tests » Compatibility with Metatag Fields (and test coverage)

I just ran into this issue that metatag fields do not appear to be supported on the latest 8.x-2.x, Metatag 1.8.0, and Drupal 8.7.5.

To reproduce:

  • Install Metatag
  • Create a new empty content type (just with Body field).
  • Add a Metatag field to that content type.
  • Set up Entity Share to share that content type.
  • Pull in a sample piece of content.

The batch process to pull in the content fails with the following error:

Notice: Undefined index: description in Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize() (line 130 of /app/drupal8/web/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php).

Relevant part of the backtrace:

array:34 [▼
  "33: Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize()" => array:2 [▼
    "file" => "core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php:130"
    "args" => array:4 [▼
      0 => array:1 [▶]
      1 => "Drupal\metatag\Plugin\Field\FieldType\MetatagFieldItem"
      2 => "api_json"
      3 => array:4 [▶]
    ]
  ]
  "32: Symfony\Component\Serializer\Serializer->denormalize()" => array:2 [▶]
  "31: Drupal\jsonapi\Serializer\Serializer->denormalize()" => array:2 [▶]
  "30: Drupal\jsonapi\Normalizer\FieldNormalizer->denormalize()" => array:2 [▶]
  "29: Symfony\Component\Serializer\Serializer->denormalize()" => array:2 [▶]
  "28: Drupal\jsonapi\Serializer\Serializer->denormalize()" => array:2 [▶]
  "27: Drupal\jsonapi\Normalizer\ContentEntityDenormalizer->prepareInput()" => array:2 [▶]
  "26: Drupal\jsonapi\Normalizer\EntityDenormalizerBase->denormalize()" => array:2 [▶]
  "25: Symfony\Component\Serializer\Serializer->denormalize()" => array:2 [▶]
  "24: Drupal\jsonapi\Serializer\Serializer->denormalize()" => array:2 [▶]
  "23: Drupal\jsonapi\Normalizer\JsonApiDocumentTopLevelNormalizer->denormalize()" => array:2 [▶]
  "22: Drupal\entity_share_client\Service\JsonapiHelper->extractEntity()" => array:2 [▶]
  "21: Drupal\entity_share_client\Service\JsonapiHelper->importEntityListData()" => array:2 [▶]
  "20: Drupal\entity_share_client\JsonapiBatchHelper::importEntityListBatch()" => array:2 [▶]
  "19: _batch_process()" => array:1 [▶]
  "18: _batch_do()" => array:1 [▶]
  "17: _batch_page()" => array:2 [▶]
  "16: Drupal\system\Controller\BatchController->batchPage()" => array:2 [▶]
  "15: call_user_func_array()" => array:1 [▶]
  "14: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()" => array:1 [▶]
  "13: Drupal\Core\Render\Renderer->executeInRenderContext()" => array:2 [▶]
  "12: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()" => array:2 [▶]
  "11: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()" => array:1 [▶]
  "10: Symfony\Component\HttpKernel\HttpKernel->handleRaw()" => array:2 [▶]
  " 9: Symfony\Component\HttpKernel\HttpKernel->handle()" => array:2 [▶]
  " 8: Drupal\Core\StackMiddleware\Session->handle()" => array:2 [▶]
  " 7: Drupal\Core\StackMiddleware\KernelPreHandle->handle()" => array:2 [▶]
  " 6: Drupal\page_cache\StackMiddleware\PageCache->pass()" => array:2 [▶]
  " 5: Drupal\page_cache\StackMiddleware\PageCache->handle()" => array:2 [▶]
  " 4: Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()" => array:2 [▶]
  " 3: Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()" => array:2 [▶]
  " 2: Stack\StackedHttpKernel->handle()" => array:2 [▶]
  " 1: Drupal\Core\DrupalKernel->handle()" => array:2 [▶]
  " 0: main()" => array:2 [▶]
]

Any suggestions for a work-around?

Grimreaper’s picture

For a work-around, I think you can use JSON:API Extras to disable the metatag field in the endpoint.

Grimreaper’s picture

Priority: Normal » Major
DeFr’s picture

Grimreaper’s picture

Thanks @DeFr,

That will help me to find a patch to test!

DeFr’s picture

You're welcome :-) Testing that issue patch and then reading its history though, it's mainly focusing for now on correctly exposing the actual meta tags to JSON:API . That's interesting for Entity Share (because right now if the meta configuration isn't the same in the site you're pulling from, you may get different meta in the end) but that wasn't the blocking point for me at first.

Part of the issue got fixed for me by upgrading JSON:API to 8.7.8 ; with #3048348: Denormalizing NULL for an optional @FieldType=address or @FieldType=geolocation field fails due to either no main property name or computed read-only main property applied, entities that don't have explicit meta tags are now correctly imported.

For entities that do have meta tags explicitely set though, as far as i can tell no work was started to ensure that Metatag properties can be property denormalized. It was mentionned in #2945817-41: Support JSON API, REST, GraphQL and custom normalizations via new computed field but as far as I can tell never went further.

Grimreaper’s picture

Assigned: Unassigned » Grimreaper
Grimreaper’s picture

Rebasing patch from comment 4.

Grimreaper’s picture

Status: Needs work » Needs review

@DeFr,

On Core 8.7.8, I also can now import entities without explicit metatags, only tokens (because nothing is exposed in the JSON of JSON:API).

I will trigger the tests to be able to merge the patch adding the metatag field on the test content type, so no more need to reroll that.

Status: Needs review » Needs work
Grimreaper’s picture

Arf,

I won't be able to merge the patch because of deprecation warnings in other modules.

I have tested the patch from #2945817-73: Support JSON API, REST, GraphQL and custom normalizations via new computed field, It transforms the JSON from:

"metatag": null,
"field_es_test_metatag": {
                    "abstract": "abstract"
                },

into:

"metatag": null,
                "metatag_normalized": [
                    {
                        "tag": "meta",
                        "attributes": {
                            "name": "title",
                            "content": "Test 1 | Drupal 8"
                        }
                    },
                    {
                        "tag": "link",
                        "attributes": {
                            "rel": "canonical",
                            "href": "http://site1.web.entity-share.docker.localhost/node/1"
                        }
                    },
                    {
                        "tag": "meta",
                        "attributes": {
                            "name": "abstract",
                            "content": "abstract"
                        }
                    }
                ],
"field_es_test_metatag": {
  "abstract": "abstract"
},

To get the value obtained by replacing tokens by the real values, metatag_normalized will be good, but maybe hardly useable.

And as metatag and metatag_normalized are computed fields, I think it will be necessary to unset them in JsonapiHelper::prepareEntityData().

I am going to test to make a field enhancer on the metatag field to get the values computed with tokens and to serialize the value as it is what is expected by the metatag field.

Grimreaper’s picture

Assigned: Grimreaper » Unassigned
Status: Needs work » Needs review
FileSize
6.7 KB

I have made a separated patch from the patch that adds the metatag field to not have deprecation warnings from other modules.

It adds an enhancer that should be used on both client and server.

@quicksketch and @DeFr, I will wait for your feedbacks before writing automated tests cases.

Can you also give me your tests scenario?

So if the tests will be ok, I will be able to merge the patch adding the enhancer and have the patch adding the metatag field on the test content type and the automated tests later in a postponed state.

DeFr’s picture

Status: Needs review » Needs work

On my side, that patch doesn't really seems to work.

That being said, if I change the field enhancer to return ['value' => serialize($data)]; instead of return $data; , then things look better.

Tested with the 'metatag' computed base field disabled through JSON:API Extra, and field_meta_tags configured with the new field enhancer at the default values, with a node that has actual explicit meta defined. If I disable the default tokens, it seems to be broken both with the explicit serialize() call and without.

Grimreaper’s picture

Thanks @DeFr for the feedback.

Strange, it worked for me.

I will need to create an automated tests to prove that it works (or not).

DeFr’s picture

Status: Needs work » Needs review
FileSize
656 bytes
6.1 KB

OK, so after some discussion on Slack, the fail was due to me only setting up the enhancer on the server and not on the client. If I set both instances with the enhancer, and leave the token replacement in place, everything works fine with the patch, which is quite cool !

That being said, if I uncheck the default tokens on the server field enhancer, I get an exception thrown in JSON:API : Drupal\jsonapi_extras\Plugin\ResourceFieldEnhancerBase returned invalid output data: [] .

With the attached patch, I'm both getting rid of the exception (there's always something to serialize) and the need to setup the enhancer on the client.

Grimreaper’s picture

Assigned: Unassigned » Grimreaper

Thanks @DeFr for the feedback.

I will write some automated tests to be clear of the results.

Grimreaper’s picture

Assigned: Grimreaper » Unassigned

Unassigning as #3087251: Remove (or add) trailing slash on remote URLs for consistency is more difficult than expected.

Will be back on this issue after.

Grimreaper’s picture

Assigned: Unassigned » Grimreaper
Grimreaper’s picture

Assigned: Grimreaper » Unassigned
FileSize
21.94 KB
+++ b/src/Plugin/jsonapi/FieldEnhancer/MetatagEnhancer.php
@@ -0,0 +1,182 @@
+      'value' => $data

missing comma for CS ;)

Here is a new patch that takes the field enhancer from comment 21.

I have renamed the plugin entity_share_metatag to be able to commit it in Entity share to unblock the issue while discussing if it should go in JSON:API Extras or Metatag.

Also the test will fail due to deprecation warnings from Metatag.

The goal is to be ok with the test cases.

Thanks for the review.

Status: Needs review » Needs work

The last submitted patch, 25: entity_share-metatag_enhancer-3060702-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Grimreaper’s picture

Status: Needs work » Needs review
quicksketch’s picture

I'm getting errors when testing this patch. Here's what I did:

- Applied the patch to the latest 2.x
- Configured JSON API Extras to re-enabled our metatag field (field_meta).
- Enabled the Entity Share Enhancer on the metatag field, with all 3 options (the defaults) enabled.
- Did this on both the client and provider site.
- Visited the client pull form, but it will no longer load the list of content to pull. There is an HTTP 500 error. On the source site, the following entries are made in watchdog:

Notice: Undefined index: field_item_object in Drupal\entity_share\Plugin\jsonapi\FieldEnhancer\EntityShareMetatagEnhancer->doUndoTransform()

Error: Call to a member function getEntity() on null in Drupal\entity_share\Plugin\jsonapi\FieldEnhancer\EntityShareMetatagEnhancer->doUndoTransform()

It seems as though the latest patch may have a hard-coded field name in it? field_item_object?

quicksketch’s picture

For reference, our use case for this feature is as follows:

- On the Source content, we have 3 fields shown on the node form: Description, Prevent search engines (noindex), and Prevent Following Links (nofollow).
- All other fields are predefined by tokens, for things like og:image, and (most importantly) canonical_url.
- When the client pulls down, we want to make sure that these token-based values are converted in the cloned copy, e.g. og:image and canonical URLs should be of the source URLs, not that of the client site.

From the looks of the UI and code, it seems as though this is what the converter intends to do. Even though I haven't gotten it working yet, it looks promising!

Grimreaper’s picture

Thanks @quicksketch for the feedbacks.

The steps are ok and the feature you need will be ok with the enhancer.

field_item_object is not a hardcoded field machine name, it is added by jsonapi_extras https://git.drupalcode.org/project/jsonapi_extras/blob/8.x-3.x/src/Norma...

Before calling undoTransform method, so it should be present.

Just to be sure, on which version of Core and jsonapi_extras are you?

quicksketch’s picture

Ah! Geez, yeah all I had to do was update JSONAPI Extras from 3.10 to 3.12. Now it works.

Now that I have set up a Metatag field to sync in JSONAPI Extras, the metatags come in across the two sites. However, since the metatag field is different between the two sites, immediately after syncing any content, it says that "Entities not synchronized" on the items just synced. I'm guessing that the metatag content differences between sites is causing this.

So now this is rather tricky. We want JSONAPI to transform the metatag data when it comes in while importing, but if comparing two entities to determine if they're in sync, we want those fields to be ignored.

quicksketch’s picture

I'm having trouble reproducing my own report now... I think this seems great for the time being. If I manage to recreate the problem maybe we can do that in a follow-up.

Grimreaper’s picture

Thanks @quicksketech for the feedback.

but if comparing two entities to determine if they're in sync, we want those fields to be ignored.

The comparison is still done on the changed timestamp. so it is mismatching changed timestamp the problem.

If you can give exact steps to reproduce that will be great to be able to have an automated test and a fix about the case because I already have a test about this comparison and it is ok but maybe there are more complex case in which it is not ok.

Is the patch ok for you?

I can commit only the enhancer as automated tests will fail due to deprecation warnings in Metatag.

quicksketch’s picture

Status: Needs review » Fixed

Yes, I think this works great. Let's go ahead with it. :)

Also, kudos for the clever solution provided here. Metatag has some complicated ways of storing data: sometimes tokens and somtimes hand-types strings. The option to expand the tokens for the purposes of JSONAPI is very cool. Seems like something that might be useful for Metatag itself at some point.

quicksketch’s picture

Status: Fixed » Reviewed & tested by the community

  • Grimreaper authored 63f7282 on 8.x-2.x
    Issue #3060702 by Grimreaper, DeFr, gidel, quicksketch: Compatibility...
Grimreaper’s picture

Status: Reviewed & tested by the community » Fixed

Follow up issue created for the tests: #3089844: Tests: Metatag field.

I will update the project page.

Grimreaper’s picture

Title: Compatibility with Metatag Fields (and test coverage) » Compatibility with Metatag Fields

Status: Fixed » Closed (fixed)

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

Diveshh’s picture

Do you guys know how to get this working?
I have enabled the field enhancers on both the client & the server in the node configuration of the JSON API Extras but I still get the Ajax error.

Any help would be highly appreciated.

I'm on Drupal 8.7.10 with JSON API Extras 3.13 and Entity Share Alpha9 installed.

Musa.thomas’s picture

still got this issue :

Message Error : Call to a member function getClass() on null dans Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize() (/var/www/html/docroot/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php ligne 130)
Message Notice : Undefined index: title dans Drupal\jsonapi\Normalizer\FieldItemNormalizer->denormalize() (/var/www/html/docroot/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php ligne 130)

Drupal core 8.9.11
json api extras 3.19
entity share 3.0.0-beta4

Try to export custom content entity with paragraph and node reference.

Try to disable metatag field on serveur, nothing change.

Piotr Pakulski’s picture

Hi,
cc @Musa.thomas @Diveshh
I run into the same problem with `Call to a member function getClass()`
Was able to fix by using two custom patches - one for jsapi one for entity_share.
Attaching them I'm open to any suggestions.

dgroene’s picture

Also experiencing this issue, despite using the metatag data enhancer on both client and server. Wondering if this ticket should be reopened, or else a new one created?

Oscaner’s picture

Oscaner’s picture

FileSize
2.54 KB
985 bytes
Oscaner’s picture

FileSize
2.56 KB
1.42 KB
Grimreaper’s picture

Hello,

This issue is closed, so please create a new one with detailed steps on how to reproduce or even better, a failing test.

guaneagler’s picture

Just add a new version of file "jsonapi-metatag_enhancer-3060702-42.patch" on #42 for drupal 9.5

soorry, the file name should be *-48