Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#73 | 2916252-73.patch | 9.56 KB | Alex Bukach |
#72 | 2916252-72.patch | 9.56 KB | Akhil Babu |
#66 | 2916252-66.patch | 9.55 KB | lawxen |
#58 | interdiff-2916252-57-58.txt | 450 bytes | lawxen |
#58 | 2916252-58.patch | 1.98 KB | lawxen |
Comments
Comment #2
lawxen CreditAttribution: lawxen at Sparkpad commentedAdjustment's amount stores object Price, but Price can't be normalized either, so I make Drupal\commerce_price\ implements TypedDataInterface too.
Comment #3
bojanz CreditAttribution: bojanz at Centarro commentedThat 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.
Comment #4
lawxen CreditAttribution: lawxen at Sparkpad commented@bojanz
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
So we can just make the object Adjustment and Price to be traversable.
Comment #5
skyredwang#4 Looks worse. Why do we need to change Price at all? It has been working with serialization on Product Variant and Order.
Comment #6
skyredwang#4 Looks worse. Why do we need to change Price at all? It has been working with serialization on Product Variant and Order.
Comment #7
lawxen CreditAttribution: lawxen at Sparkpad commented@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
Comment #8
lawxen CreditAttribution: lawxen at Sparkpad commentedProduct 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:
Comment #9
skyredwang#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?Comment #10
Wim LeersComment #11
Wim LeersComment #12
Wim LeersAFAICT 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 inOrder
.Comment #13
skyredwang@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.
Comment #14
bojanz CreditAttribution: bojanz at Centarro commentedCorrect. 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.
Comment #15
lawxen CreditAttribution: lawxen at Sparkpad commented#9's question:
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:
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!
1. https://www.drupal.org/node/2915705
Core change: TypedDataNormalizer.php->normalzie():
Then the code will run to symfony's Serializer->normalize()
Let's focused on
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
Comment #16
lawxen CreditAttribution: lawxen at Sparkpad commentedThere is a new solution,
change TypedDataNormalizer->normalize() to use (array) to translate object;
But It can't handle two situations:
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
Comment #17
skyredwang@bojanz has told me on IRC:
Comment #18
lawxen CreditAttribution: lawxen at Sparkpad commentedon 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.
Comment #19
lawxen CreditAttribution: lawxen at Sparkpad commented#17 suggest add custom normalizer on field level with no changing for Adjustment
This is the patch
Comment #20
skyredwangI tested #19, it seems working fine, below is a sample output, you can see the adjustments are serialized:
Comment #21
lawxen CreditAttribution: lawxen at Sparkpad commentedfix the comment
@see \Drupal\commerce\Normalizer\ObjectAnyNormalizer::supportsNormalization
to
@see \Drupal\commerce_order\Normalizer\ObjectAnyNormalizer::supportsNormalization
Comment #22
mglamanNit: Can't this be `commerce_order.normalizer`. It's a normalizer and not provided by the Serializer module.
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.
Comment #23
bojanz CreditAttribution: bojanz at Centarro commentedcommerce_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?
Comment #24
mglamanSorry I meant `commerce_order.normalizer.adjustment` not just `commerce_order.normalizer`. Was regarding to the namespace of the service.
Comment #25
lawxen CreditAttribution: lawxen at Sparkpad commentedisset($value) && is_object($value) && $value instanceof Adjustment
to$value instanceof Adjustment
serializer.normalizer.any_adjustment:
tocommerce_order.normalizer.adjustment
Comment #26
mglamanI think this comment is outdated and should be removed, correct?
Test failures look to be due to the random coupon redemption test failures.
Comment #27
lawxen CreditAttribution: lawxen at Sparkpad commentedRemove 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
Comment #28
lawxen CreditAttribution: lawxen at Sparkpad commentedComment #29
mglamanThis looks good to me! Just want to check with bojanz
Comment #30
lawxen CreditAttribution: lawxen at Sparkpad commentedIf wget https://www.drupal.org/files/issues/2916252-27.patch to local, It display
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.
Comment #31
mglamancaseylau thanks! I'll commit this today.
Comment #32
Wim LeersIf we're just using priority 0, then we can omit both the comment and the service priority.
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.
Oh but this then does the necessary extra checking. Why not just declare
Drupal\commerce_order\Adjustment
the supported class?Looks good, but I don't see an equivalent
denormalize()
method?This means only
GET
will work,PATCH
andPOST
will still fail.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.Comment #34
mglamanCommitted, thanks!
Comment #35
mglamanPutting to needs work. Totally missed #32. I am not reverting. Let's just make a new patch which
denormalize
dataComment #36
lawxen CreditAttribution: lawxen at Sparkpad commentedFirst reply #32 @Wim Leers:
This is right
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();
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
So this is wrong
This is right, denormalize is needed.
please create a functional test for this.
This is right, functional test is needed too.
Patch:
Two patch's Pros and cons:
pros:
cons:
pros:
cons:
Besides:
Upload a normalize and denormalize flow debug image:
Comment #37
mglamanThis is above my head, now, so I'll see what Wim says :) Patch A looks to be my preference as well.
I think hacking into AdjustmentItem here is not the end of the world. Just because we are supporting non-TypedData value objects.
Comment #40
drugan CreditAttribution: drugan as a volunteer commentedSeems 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
Comment #41
mglamanArgh, 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)
Comment #42
drugan CreditAttribution: drugan as a volunteer commentedMy 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
Comment #43
drugan CreditAttribution: drugan as a volunteer commentedOK, 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.
Comment #44
mglamandrugan, 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.
Comment #46
mglamanOkay, 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.
Comment #47
lawxen CreditAttribution: lawxen at Sparkpad commented@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
@drugan Can you direct me reproduce the bug?
Comment #48
lawxen CreditAttribution: lawxen at Sparkpad commentedOh, Even if I can't reproduce the bug, we should do
Because it use serialization's code
Comment #49
drugan CreditAttribution: drugan as a volunteer commentedIt 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 withdrush 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: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.Comment #50
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #51
lawxen CreditAttribution: lawxen at Sparkpad commentedComment #52
skyredwangThis 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.
Comment #53
Wim LeersSo 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:
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 :)
Comment #54
lawxen CreditAttribution: lawxen at Sparkpad commentedCombined #7 #51 #53
like #53 described.
prepare for #2915705: TypedData 'Any' can't be normalized to array if it stores an object
Add denormalize
Comment #55
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #56
lawxen CreditAttribution: lawxen at Sparkpad commentedJust a re-roll, Nothing change.
Comment #57
lawxen CreditAttribution: lawxen at Sparkpad commentedComment #58
lawxen CreditAttribution: lawxen at Sparkpad commentedComment #59
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedI 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.
Comment #62
mglamanIt's postponed because core should fix the problem itself :/ The patches are just here as a workaround.
Comment #63
mglaman#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)
Comment #64
Wim LeersYep, and this still needs test coverage :)
See the many subclasses of
EntityResourceTestBase
for examples!Comment #65
lawxen CreditAttribution: lawxen at Sparkpad commented#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.
I have tested that 2916252-58.patch can't be fix this issue alone, I will write a test to prove it.
Comment #66
lawxen CreditAttribution: lawxen at Sparkpad commentedAdd resource test for adjustment field.
Comment #67
lawxen CreditAttribution: lawxen at Sparkpad commentedComment #68
Wim LeersThanks for working on test coverage! Now let's get it to pass :)
Comment #69
mglamanRerunning the tests to see where this is at :)
Comment #70
mglamanI missed where this is blocked on #2915705: TypedData 'Any' can't be normalized to array if it stores an object.
Comment #71
yonailo CreditAttribution: yonailo commentedI 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.
Comment #72
Akhil BabuThanks 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
to
Comment #73
Alex Bukach CreditAttribution: Alex Bukach at Oomph, Inc. commentedIt should be
\Traversable
, not\ArrayIterator
.Comment #74
Agence Web CoherActio CreditAttribution: Agence Web CoherActio commentedThe last patch caused side-effects (e.g. price substract() method with adjustment).
I ended up extending Drupal TypedDataNormalizer class to support Ajdustment normalizer