Problem/Motivation

Order's Adjustment can't be normalized, So the field adjustments can't been serialized
there is a core bug cause this, #2915705: TypedData 'Any' can't be normalized to array if it stores an object
the core will not recursive normalzie object,

But the resolution require that object itself saved to a 'any' type data must been normalizable or traversable

Proposed resolution

Make Adjustment been traversable

Remaining tasks

make Adjustment implement IteratorAggregate

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#73 interdiff-2916252-66-73.txt388 bytesAlex Bukach
#73 2916252-73.patch9.56 KBAlex Bukach
#72 2916252-72.patch9.56 KBAkhil Babu
#66 interdiff-2916252-58-66.txt6.97 KBlawxen
#66 2916252-66.patch9.55 KBlawxen
#58 interdiff-2916252-57-58.txt450 byteslawxen
#58 2916252-58.patch1.98 KBlawxen
#57 interdiff-2916252-56-57.txt549 byteslawxen
#57 2916252-57.patch1.67 KBlawxen
#56 interdiff-2916252-54-56.txt1.25 KBlawxen
#56 2916252-56.patch1.39 KBlawxen
#54 interdiff-2916252-53-54.txt2.02 KBlawxen
#54 2916252-54.patch2.13 KBlawxen
#53 2916252-53.patch1.03 KBWim Leers
#51 2916252-51.patch12.69 KBlawxen
#36 2916252-36-a.patch7.97 KBlawxen
#36 2916252-36-b.patch10.25 KBlawxen
#36 normalze-denormalize-flow.png1.54 MBlawxen
#30 2916252-30.patch5.3 KBlawxen
#30 interdiff-2916252-25-30.txt553 byteslawxen
#27 2916252-27.patch5.3 KBlawxen
#27 interdiff-2916252-25-27.txt553 byteslawxen
#25 2916252-25.patch5.39 KBlawxen
#25 interdiff-2916252-21-25.txt4.34 KBlawxen
#21 2916252-21.patch2.5 KBlawxen
#21 interdiff-2916252-19-21.txt630 byteslawxen
#19 2916252-19.patch2.49 KBlawxen
#19 interdiff-2916252-18-19.txt5.25 KBlawxen
#18 2916252-18.patch3.38 KBlawxen
#18 interdiff-2916252-7-18.txt2.17 KBlawxen
#7 interdiff-2916252-4-7.txt863 byteslawxen
#7 2916252-7.patch1.04 KBlawxen
#4 interdiff-2916252-1-4.txt4.33 KBlawxen
#4 2916252-4.patch1.6 KBlawxen
#2 2916252-1.patch3.82 KBlawxen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

caseylau created an issue. See original summary.

lawxen’s picture

Adjustment's amount stores object Price, but Price can't be normalized either, so I make Drupal\commerce_price\ implements TypedDataInterface too.

bojanz’s picture

That patch looks very wrong. Adjustment and Price are dumb value objects, we should not pollute them with a dozen Drupal-specific methods. The conversion needed for normalization/serialization should ideally happen one level above, at the field type level.

lawxen’s picture

@bojanz

That patch looks very wrong. Adjustment and Price are dumb value objects, we should not pollute them with a dozen Drupal-specific methods

I agree this
meanwhile the normalization/serialization shouldn't need function change the basic logic

Combine https://www.drupal.org/node/2915705‘s patch
The code will run to here
https://github.com/symfony/serializer/blob/3.2/Serializer.php#L149

        if (is_array($data) || $data instanceof \Traversable) {
            $normalized = array();
            foreach ($data as $key => $val) {
                $normalized[$key] = $this->normalize($val, $format, $context);
            }

So we can just make the object Adjustment and Price to be traversable.

skyredwang’s picture

#4 Looks worse. Why do we need to change Price at all? It has been working with serialization on Product Variant and Order.

skyredwang’s picture

#4 Looks worse. Why do we need to change Price at all? It has been working with serialization on Product Variant and Order.

lawxen’s picture

@skyredwang yes, you are right
I make some change
1. don't change Price
2.on Adjustment's toArray(), return the Price's toArray() value

lawxen’s picture

Product Variant and Order don't storage Price object ,It just storage Price's value and currency, But “don't change Price” still is right,
It has some benefit:

  1. If Adjustment add another object tomorrow, It make an example of just call new object's toArray() function, no change for the new object is needed always
  2. Faster
skyredwang’s picture

#7 looks cleaner, but #3 question still remains: Can we do serialization at the field level? Specifically speaking, why can't we normalize $Order->adjustments? One reason I can think of is that Adjustment uses 'Any' for storage, therefore on the field level, there isn't metadata we can utilize to normalize $Order->adjustments, therefore, we need to go to one level deeper.

@caseylau, can you prove or disapprove that $Order->adjustments can be serialized at field level?

Wim Leers’s picture

Issue tags: +API-First Initiative
Wim Leers’s picture

Wim Leers’s picture

AFAICT the real problem here is that Adjustment doesn't have a proper @DataType. It's not clear to me why it doesn't at least use \Drupal\Core\TypedData\Plugin\DataType\Map in its base field definition at in Order.

skyredwang’s picture

It's not clear to me why it doesn't at least use \Drupal\Core\TypedData\Plugin\DataType\Map in its base field definition at in Order

@bojanz has told me: this is because when they started doing Adjustment, they couldn't decide on a clear data structure for Adjustment. Core has 'inferior' support on field type change (or something like that), therefore, there isn't a plan to change it for now.

bojanz’s picture

Correct. Adding a column to an existing field type is hell (which we've had to go through with Address, but wanted to avoid here). Adjustments changed multiple times. Since we only needed a serialized, non-queriable solution, the current approach made sense.

lawxen’s picture

#9's question:

can you prove or disapprove that $Order->adjustments can be serialized at field level?

Because all Adjustment's properties are protected, and Drupal can't konw it's get*** function,

So there are ony two possible plan as far as know:

  1. Write Custom normalizer in commerce itself, to make Adjustment can be normalzed on field level
  2. Custom normalizer use Adjustment's several get*** function to get value;
    If Adjustment change, the normalizer need change too. And every situation like this need a standalone normalizer.
    So many garbage codes!

  3. Take full advantage of symfony Serializer->normalize() design, Make Adjustment traversable(#7's solution 2916252-7.patch).

    1. https://www.drupal.org/node/2915705
    Core change: TypedDataNormalizer.php->normalzie():

    -    return $object->getValue();
    +    $value = $object->getValue();
    +    // If the value is object or array, return the normalized result, object
    +    // must be typed data or traversable to be normalizable.
    +    if (isset($value) && (is_object($value) || is_array($value))) {
    +      $value = $this->serializer->normalize($value, $format, $context);
    +    }
    +    return $value;
    

    Then the code will run to symfony's Serializer->normalize()

       public function normalize($data, $format = null, array $context = array())
        {
            // If a normalizer supports the given data, use it
            if ($normalizer = $this->getNormalizer($data, $format)) {
                return $normalizer->normalize($data, $format, $context);
            }
    
            if (null === $data || is_scalar($data)) {
                return $data;
            }
    
            if (is_array($data) || $data instanceof \Traversable) {
                $normalized = array();
                foreach ($data as $key => $val) {
                    $normalized[$key] = $this->normalize($val, $format, $context);
                }
    
                return $normalized;
            }
    
            if (is_object($data)) {
                if (!$this->normalizers) {
                    throw new LogicException('You must register at least one normalizer to be able to normalize objects.');
                }
    
                throw new UnexpectedValueException(sprintf('Could not normalize object of type %s, no supporting normalizer found.', get_class($data)));
            }
    
            throw new UnexpectedValueException(sprintf('An unexpected value could not be normalized: %s', var_export($data, true)));
        }
    

    Let's focused on

    instanceof \Traversable
    

    If Adjustment is Traversable, it wll loop Adjustment to get it value.
    So just make Adjustment implements \IteratorAggregate to make it Traversable
    #7's 2916252-7.patch

  1. We can see that solution one:Write Custom normalizer is boring and chaos, result in a lot of similar codes in Drupal. So normalize Adjustment through custom normalizer on field level is not recommend.
  2. And the solution two use symfony's design to make things easier and clear.(recomend)
lawxen’s picture

There is a new solution,
change TypedDataNormalizer->normalize() to use (array) to translate object;

  public function normalize($object, $format = NULL, array $context = []) {
    $value =  $object->getValue();
    if (isset($value) && is_object($value))
    $value = (array) $value;
    //then write code to remove * on protected value's key
    return $value;
  }

But It can't handle two situations:

  1. Situation here on Adjustment , (array) can't return object's object value, like object Price stored on Adjustment->amount
  2. 'any' typed data stored an array with child object value
    If we want handle this situation, we must loop the array on every level's child element, it's inefficient.

conclusion:
(array) is not recommend

skyredwang’s picture

Status: Active » Needs work

@bojanz has told me on IRC:

<bojanz> skyred: lets just write a custom serializer then
<bojanz> that way we can avoid patching core
<skyred> bojanz: this custom normalizer, do you want it to be in Commerce project, or you want it to be in a contrib?
<bojanz> skyred: in commerce itself
lawxen’s picture

<bojanz> skyred: lets just write a custom serializer then
<bojanz> that way we can avoid patching core

on the basis of your suggestion. before https://www.drupal.org/node/2915705 be committed in core
I add a custom normalizer, naming it a more generic name : ObjectAnyNormalizer, to make it can support more similar situations through this one normalizer in commerce.

lawxen’s picture

#17 suggest add custom normalizer on field level with no changing for Adjustment
This is the patch

skyredwang’s picture

Status: Needs work » Needs review

I tested #19, it seems working fine, below is a sample output, you can see the adjustments are serialized:

{
    "data": {
        "type": "commerce_order--default",
        "id": "66c5f00b-530e-4410-8a18-4c49e7deb3c3",
        "attributes": {
            "order_id": 20,
            "uuid": "66c5f00b-530e-4410-8a18-4c49e7deb3c3",
            "order_number": "20",
            "mail": "test@test.com",
            "ip_address": "172.17.0.1",
            "adjustments": [
                {
                    "type": "custom",
                    "label": "adjustment",
                    "amount": {
                        "number": "4.32",
                        "currency_code": "CNY"
                    },
                    "percentage": null,
                    "sourceId": "custom",
                    "included": false,
                    "locked": true
                },
                {
                    "type": "promotion",
                    "label": "Custom promotion",
                    "amount": {
                        "number": "-5.00",
                        "currency_code": "CNY"
                    },
                    "percentage": null,
                    "sourceId": "custom",
                    "included": false,
                    "locked": false
                },
                {
                    "type": "shipping",
                    "label": "Deliver fee",
                    "amount": {
                        "number": "12.0",
                        "currency_code": "CNY"
                    },
                    "percentage": null,
                    "sourceId": "custom",
                    "included": false,
                    "locked": false
                }
            ],
            "total_price": {
                "number": "6097.160000",
                "currency_code": "CNY"
            },
            "state": "completed",
            "data": null,
            "locked": null,
            "created": 1480914436,
            "changed": 1508399562,
            "placed": 1480916496,
            "completed": 1480916496,
........
lawxen’s picture

fix the comment
@see \Drupal\commerce\Normalizer\ObjectAnyNormalizer::supportsNormalization
to
@see \Drupal\commerce_order\Normalizer\ObjectAnyNormalizer::supportsNormalization

mglaman’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
  1. +++ b/modules/order/commerce_order.services.yml
    @@ -62,3 +62,10 @@ services:
    +  serializer.normalizer.any_adjustment:
    

    Nit: Can't this be `commerce_order.normalizer`. It's a normalizer and not provided by the Serializer module.

  2. +++ b/modules/order/src/Normalizer/AnyAdjustmentNormalizer.php
    @@ -0,0 +1,51 @@
    +      if (isset($value) && is_object($value) && $value instanceof Adjustment) {
    

    This can be simplified to just $value instanceof Adjustment

Can we just add a kernel test which normalizes a field and asserts the array output? Then it looks good to me. But we cannot commit this without tests.

bojanz’s picture

commerce_order.normalizer doesn't make sense since it doesn't normalize orders, it normalizes adjustments.
So, commerce_order.adjustment_normalizer, unless a service ID pattern is enforced somehow?

mglaman’s picture

Sorry I meant `commerce_order.normalizer.adjustment` not just `commerce_order.normalizer`. Was regarding to the namespace of the service.

lawxen’s picture

  1. changeisset($value) && is_object($value) && $value instanceof Adjustment to $value instanceof Adjustment
  2. rename serializer.normalizer.any_adjustment: to commerce_order.normalizer.adjustment
  3. add AdjustmentItemNormalizeTest
mglaman’s picture

+++ b/modules/order/commerce_order.services.yml
@@ -62,3 +62,10 @@ services:
+    # @see \Drupal\commerce_order\Normalizer\ObjectAnyNormalizer::supportsNormalization

I think this comment is outdated and should be removed, correct?

Test failures look to be due to the random coupon redemption test failures.

lawxen’s picture

Remove outdated comment:
# @see \Drupal\commerce_order\Normalizer\ObjectAnyNormalizer::supportsNormalization

@mglaman #25:2916252-25.patch tests succeed at the second time when choosing the same conditons.
PHP 7.1 & MySQL 5.5, D8.4

lawxen’s picture

Status: Needs work » Needs review
mglaman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

This looks good to me! Just want to check with bojanz

lawxen’s picture

If wget https://www.drupal.org/files/issues/2916252-27.patch to local, It display

+    # This normalizer must just be higher than serializer.normalizer.typed_data,
+    # So setting priority as 0 here.

But review on the web, it display
+ # This normalizer must just be higher than serializer.normalizer.typed_data, So setting priority as 0 here.

Something wrong with drupal'org. Maybe caused by updaloading the 2916252-27.patch twice when edit the comment.

I reupload the patch no mater whatever happens.

mglaman’s picture

Assigned: lawxen » mglaman

caseylau thanks! I'll commit this today.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
  1. +++ b/modules/order/commerce_order.services.yml
    @@ -62,3 +62,9 @@ services:
    +    # This normalizer must just be higher than serializer.normalizer.typed_data,
    +    # So setting priority as 0 here.
    +      - { name: normalizer, priority: 0}
    

    If we're just using priority 0, then we can omit both the comment and the service priority.

  2. +++ b/modules/order/src/Normalizer/AnyAdjustmentNormalizer.php
    @@ -0,0 +1,51 @@
    +class AnyAdjustmentNormalizer extends NormalizerBase {
    ...
    +  protected $supportedInterfaceOrClass = 'Drupal\Core\TypedData\Plugin\DataType\Any';
    

    This would affect not just the Commerce Order module, but any entity type that uses a \Drupal\Core\TypedData\Plugin\DataType\Any.

    IOW: this introduces unwanted side effects.

  3. +++ b/modules/order/src/Normalizer/AnyAdjustmentNormalizer.php
    @@ -0,0 +1,51 @@
    +  public function supportsNormalization($data, $format = NULL) {
    +    /* @var \Drupal\Core\TypedData\Plugin\DataType\Any $data */
    +    if (parent::supportsNormalization($data, $format)) {
    +      $value = $data->getValue();
    +      if ($value instanceof Adjustment) {
    +        return TRUE;
    +      }
    +    }
    +    return FALSE;
    +  }
    

    Oh but this then does the necessary extra checking. Why not just declare Drupal\commerce_order\Adjustment the supported class?

  4. +++ b/modules/order/src/Normalizer/AnyAdjustmentNormalizer.php
    @@ -0,0 +1,51 @@
    +  public function normalize($object, $format = NULL, array $context = []) {
    

    Looks good, but I don't see an equivalent denormalize() method?

    This means only GET will work, PATCH and POST will still fail.

  5. +++ b/modules/order/tests/src/Kernel/AdjustmentItemNormalizeTest.php
    @@ -0,0 +1,109 @@
    +class AdjustmentItemNormalizeTest extends CommerceKernelTestBase {
    

    Please, please, please create a functional test for this.

    We have \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase for that in Drupal core. (See its many subclasses.)

    See \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest for an example of a functional test specifically for a field type's normalization.

  • mglaman committed fbe8dd2 on 8.x-2.x authored by caseylau
    Issue #2916252 by caseylau, skyredwang: Order's Adjustment can't be...
mglaman’s picture

Assigned: mglaman » Unassigned
Status: Needs work » Fixed

Committed, thanks!

mglaman’s picture

Status: Fixed » Needs work

Putting to needs work. Totally missed #32. I am not reverting. Let's just make a new patch which

  • Removes priority definition and comment
  • Is able to denormalize data
  • Create a matching Functional test beyond the kernel test
lawxen’s picture

First reply #32 @Wim Leers:

  1. If we're just using priority 0, then we can omit both the comment and the service priority.

    This is right

  2. This would affect not just the Commerce Order module, but any entity type that uses a \Drupal\Core\TypedData\Plugin\DataType\Any.
    IOW: this introduces unwanted side effects.

    Because commerce suggest don't change object Adjustment and Hack core or waiting core's issue be fixed, So abandon #7's patch, Just write custom normalizer, More checking will appear in supportsNormalization();

  3. Why not just declare Drupal\commerce_order\Adjustment the supported class?
    This can not be done,What this doing is replacing core's and jsonapi's severial FieldItemNormalzer
    1. If priority just higher than Drupal\serialization\Normalizer\FieldItemNormalizer, This normalzer won't replace hal or jsonapi's FieldItemNormalzer
    2. If priority higher than any other FieldItemNormalizer, You can only follow one FieldItemNormalizer's design, Other will totally crash
    3. So this is wrong

  4. but I don't see an equivalent denormalize() method?

    This is right, denormalize is needed.

  5. please create a functional test for this.
    This is right, functional test is needed too.

Patch:

  1. 2916252-36-a.patch(suggest):
    1. add some code to AdjustmentItem's setValue() to make FieldItemNormalze's denormalize() work well
    2. remove priority and comment
    3. add denormalize test
    4. add functional test
  2. 2916252-36-b.patch(disapprove):
    1. Modify AdjustmentNormalizer to make it replacing FieldItemNormalizer when denormalizing adjustment filed
    2. set priority to 22
    3. add denormalize test
    4. add functional test

Two patch's Pros and cons:

  1. 2916252-36-a.patch(suggest):
    pros:
    1. conform to origin flow of execution
    2. After core https://www.drupal.org/node/2915705 fixed, The AdjustmentItem has no need to change

    cons:

    1. Hack AdjustmentItem
  2. 2916252-36-b.patch(disapprove):
    pros:
    1. No affection on AdjustmentItem and Adjustment itself

    cons:

    1. Maybe has some affection when other module like jsonapi use it's own FieldItemNormalzer, I haven't find definite evidence
    2. Speed a little slow than 2916252-36-a.patch

Besides:
Upload a normalize and denormalize flow debug image:

mglaman’s picture

Status: Needs work » Needs review

This is above my head, now, so I'll see what Wim says :) Patch A looks to be my preference as well.

+++ b/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php
@@ -48,6 +49,11 @@ class AdjustmentItem extends FieldItemBase {
+    // Used for denormalizing.
+    if (is_array($values)) {
+      $values['amount'] = new Price($values['amount']['number'], $values['amount']['currency_code']);
+      $values = new Adjustment($values);
+    }

I think hacking into AdjustmentItem here is not the end of the world. Just because we are supporting non-TypedData value objects.

The last submitted patch, 36: 2916252-36-b.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 36: 2916252-36-a.patch, failed testing. View results

drugan’s picture

Seems something went wrong with the latest commit.

drush cr

PHP Fatal error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in /home/drugan/public_html/drugan.dev/web/modules/commerce_core_modules/commerce/modules/order/src/Normalizer/AdjustmentNormalizer.php on line 11

Fatal error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in /home/drugan/public_html/drugan.dev/web/modules/commerce_core_modules/commerce/modules/order/src/Normalizer/AdjustmentNormalizer.php on line 11
Drush command terminated abnormally due to an unrecoverable error. [error]
Error: Class 'Drupal\serialization\Normalizer\NormalizerBase' not found in
/home/drugan/public_html/drugan.dev/web/modules/commerce_core_modules/commerce/modules/order/src/Normalizer/AdjustmentNormalizer.php,
line 11

mglaman’s picture

Argh, okay. I need to revert that commit. We need to add commerce_order.normalizer.adjustment through a ServiceProvider when the Serialization module is available.

EDIT: I'm not able to reproduce this error when rebuilding cache through Drush or the UI (Performance page, core/rebuild.php)

drugan’s picture

My DC is updated to this latest commit:

commit a31816a4f0c3ca17d7fd0b9105b049a00e90cc87
Author: TommyChris
Date: Tue Oct 24 08:53:54 2017 -0500

Issue #2918392 by TommyChris: ParseError: syntax error, unexpected '$this' (T_VARIABLE) in Composer\Autoload\includeFile() (line 5

drugan’s picture

OK, the reason is the Serialization module was not enabled. Now drush cr runs without errors.

Should not be the Serialization put in the commerce_order dependencies now? Or at least to emit somehow a warning to users? In any case commerce_base install profile need to be updated to enable the Serialization on install. I'll make a PR.

mglaman’s picture

drugan, in #41 I said we need to revert the commit if it causes a soft-dependency. However, I have not been able to reproduce and our tests did not fail. Also, it should not cause an issue unless the service is invoked, which it only should be when Serializer is enabled.

I'll be reverting later today.

  • mglaman committed 4710d6f on 8.x-2.x
    Revert "Issue #2916252 by caseylau, skyredwang: Order's Adjustment can't...
mglaman’s picture

Okay, I reverted.

We'll need to merge #30 and a patch from #36. We'll also need to only register this service when the Serialization module is installed using a ServiceProvider.

See https://github.com/drupalcommerce/commerce/blob/8.x-2.x/modules/log/src/... for an example.

lawxen’s picture

@mglaman
I test #30 patch on drupal8.5.x drupal8.4.x drupal8.4.0, drupal-composer/drupal-project, even drupal8.3.7(force enable commerce2.x), cant't reproduce #40 error. I'm wondering whether we should

register this service when the Serialization module is installed using a ServiceProvider.

@drugan Can you direct me reproduce the bug?

lawxen’s picture

Oh, Even if I can't reproduce the bug, we should do

register this service when the Serialization module is installed using a ServiceProvider.

Because it use serialization's code

drugan’s picture

It looks like a bit weird but after uninstalling the Serialization module I can't reproduce the #40 error too. Magic!

My DC story:

I've installed site using my own forks of the commerce_base, project_base and DC itself. Basically, there are no any radical changes except I install some additional modules. Also, DC core modules mapped to be downloaded into web/modules/commerce_core_modules folder. All the rest modules download as usual into web/modules/contrib folder. That's is the entire difference.

So, first I've run this and then installed site (before the serialization commit):

composer create-project --repository=https://raw.githubusercontent.com/drugan/project-base/8.x/packages.json drugan/project-base some-dir --stability dev

Everything worked great. Sometimes I merge changes on the base DC project and again, before the serialization commit there was not any issues.

Note that I had the error not only with drush cr but also with drush updb --entity-updates command. Also, when trying to visit the site page I had the same fatal error reported in red as I have all errors enabled on the site. Then I've just removed the extends control from the AdjustmentNormalizer class:

/**
 * Custom normalizer for 'any' typed data with Adjustment stored.
 */
class AdjustmentNormalizer {
// ...
}

Successfully run drush cr and tried to restore the class file. Again the same error. At this stage I've decided to check if I have this module enabled at all. And no, it was not. After enabling everything worked as expected.

bojanz’s picture

I think #41 is the right approach. We always have occasional problems if a service isn't defined in a service provider but depends on an optional module.

lawxen’s picture

  1. combine 2916252-36-a.patch and 2916252-30.patch
  2. register this service when the Serialization module is installed using a ServiceProvider
skyredwang’s picture

Status: Needs work » Needs review

This issue actually do not have a dependency on #2915705: TypedData 'Any' can't be normalized to array if it stores an object anymore, as we are normalizing $order->adjustments on the field level instead of data type level.

Wim Leers’s picture

Title: Order's Adjustment can't be normalized and serialized » [PP-1] Order's Adjustment can't be normalized and serialized
Status: Needs review » Postponed
FileSize
1.03 KB

So let's postpone this on that issue then.

I believe that once #2915705: TypedData 'Any' can't be normalized to array if it stores an object is in, all this issue would need to do is:

-final class Adjustment {
+final class Adjustment implements \IteratorAggregate {

Sample implementation attached. We'd still want the tests that the previous patches have (to ensure it actually works, and to ensure we don't regress), but all of the complexity would live in that other issue :)

lawxen’s picture

Combined #7 #51 #53

like #53 described.
prepare for #2915705: TypedData 'Any' can't be normalized to array if it stores an object

Add denormalize

    // Used for denormalization.
    if (is_array($values)) {
      $values['amount'] = new Price($values['amount']['number'], $values['amount']['currency_code']);
      $values = new Adjustment($values);
    }
bojanz’s picture

I am fine with the #53 / #54 approach, since the additions to the Adjustment class are minimal, and consist of wrapping toArray(), which is useful on its own.

lawxen’s picture

Just a re-roll, Nothing change.

lawxen’s picture

lawxen’s picture

-      'amount' => $this->amount,
+      'amount' => $this->amount->toArray(),
josephdpurcell’s picture

Status: Postponed » Needs review

I encountered this issue using Commerce 8.x-2.7 and Drupal 8.5. I have confirmed that patch #58 applies correctly and is sufficient in terms of being able to serialize adjustments.

Given that this patch applies and is working fine, is there a need to postpone this issue? Setting to "Needs Review" to ensure this doesn't go missed. The patch is quite small. I apologize in advance if this patch does in fact still need to be postponed.

The last submitted patch, 53: 2916252-53.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 58: 2916252-58.patch, failed testing. View results

mglaman’s picture

Status: Needs work » Postponed

It's postponed because core should fix the problem itself :/ The patches are just here as a workaround.

mglaman’s picture

Title: [PP-1] Order's Adjustment can't be normalized and serialized » Order's Adjustment can't be normalized and serialized
Status: Postponed » Needs work

#2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map landed in 8.6.x!

No longer postponed.

Needs work and review against the 8.6.x branch.

Will required is to force ^8.6.0 in order to land. Which means this is blocked on 8.6.0 (for official commit)

Wim Leers’s picture

Yep, and this still needs test coverage :)

See the many subclasses of EntityResourceTestBase for examples!

lawxen’s picture

Assigned: Unassigned » lawxen
Status: Needs work » Postponed

#2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map landed in 8.6.x!

No longer postponed.

#2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map has nothing relationship with this issue, And it can't fix Order's Adjustment's normalization problem with patch 2916252-58.patch .

#59 Maybe @josephdpurcell used both TypedData 'Any' can't be normalized to array if it stores an object and 2916252-58.patch.

Yep, and this still needs test coverage :)

See the many subclasses of EntityResourceTestBase for examples!

I have tested that 2916252-58.patch can't be fix this issue alone, I will write a test to prove it.

lawxen’s picture

Assigned: lawxen » Unassigned
Wim Leers’s picture

Thanks for working on test coverage! Now let's get it to pass :)

mglaman’s picture

Rerunning the tests to see where this is at :)

mglaman’s picture

Title: Order's Adjustment can't be normalized and serialized » [PP-1] Order's Adjustment can't be normalized and serialized
yonailo’s picture

I can confirm that 2916252-66.patch + 2915705-59.patch work nicely for me. I had found this issue when creating an Order Item + using the module 'content_sync'. Those 2 patches solved the issue for me.

Akhil Babu’s picture

FileSize
9.56 KB

Thanks for the patch. Adjustment values are now getting displayed in API response after applying the latest patch from https://www.drupal.org/project/drupal/issues/2915705 and then 2916252-66.patch.
But there was a waring after applying 2916252-66.patch.
Deprecated function: Return type of Drupal\commerce_order\Adjustment::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in include()
Updating 2916252-66.patch to remove the warning.
Change

  public function getIterator() {
    return new \ArrayIterator($this->toArray());
  }

to

  public function getIterator(): \ArrayIterator {
    return new \ArrayIterator($this->toArray());
  }
Alex Bukach’s picture

FileSize
9.56 KB
388 bytes

It should be \Traversable, not \ArrayIterator.

Agence Web CoherActio’s picture

The last patch caused side-effects (e.g. price substract() method with adjustment).
I ended up extending Drupal TypedDataNormalizer class to support Ajdustment normalizer

namespace Drupal\my_module\Normalizer;

use Drupal\commerce_order\Adjustment;
use Drupal\Core\TypedData\TypedDataInterface;
use Drupal\serialization\Normalizer\TypedDataNormalizer;

class MyModuleAdjustmentNormalizer extends TypedDataNormalizer {

  /**
   * {@inheritdoc}
   */
  public function normalize($object, $format = NULL, array $context = []): array|string|int|float|bool|\ArrayObject|NULL {
    $this->addCacheableDependency($context, $object);
    $value = $object->getValue();
    // Support for stringable value objects: avoid numerous custom normalizers.
    if (is_object($value) && method_exists($value, '__toString')) {
      $value = (string) $value;
    }
    if ($value instanceof Adjustment) {
      $value = $value->toArray();
    }
    return $value;
  }
}