This fixes the payment test fail. You might want to add a ConfigurablePluginInterface check somewhere, not sure if you consider configuration to be optional or not.

I'm, unsure about the approach you took. A field type that has a computed property that is the main property and the other properties are actually computed and routed through to the other one is not something I've ever seen before and I'm not sure it's a good idea.

Yes, having the other two properties not kept in sync can be a problem, but that's how everything else works, e.g. config entities with plugins. They only sync back on preSave(). The current approach requires you to fight against the default behavior, as shown by #2413471: FieldItemBase must check property definitions instead of already instantiated properties which gets in your way and this issue. There might be more.

Lets discuss this, if possible with some other field maintainers, maybe in vienna?

CommentFileSizeAuthor
#3 plugin_2620218_3.patch1.07 KBXano
#3 interdiff.txt1.29 KBXano
getValue-plugin.patch1.06 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented

Thanks! This makes sense.

The reason the plugin instance is the canonical field value, is that I wanted the data types to be independent from the field item, which means we cannot rely on ::onChange() to propagate the event properly. #2620238: Allow plugin collections to reset the contained plugin instance is the first step towards integrating the field item and its properties more tightly in order to improve the communication between the classes.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
FileSize
interdiff.txt1.29 KB
plugin_2620218_3.patch1.07 KB

I simplified the code a bit. It leads to a small performance regression if there is no plugin instance, but it's faster if there is one. I also extended the documentation a bit, as I wanted to be extremely clear why we do this. Typed Data API isn't the easiest subsystem to understand and I'd rather not have anyone, including me, remove this by accident in the future. What do you think?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Works for me too, the difference is actually quite small, considering that those two calls do the same as my patch.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Reviewed & tested by the community » Fixed

Thanks for looking into this problem!

  • Xano committed 74382dd on 8.x-2.x authored by Berdir 
    Issue #2620218 by Xano, Berdir: Override getValue() to ensure that it...

The last submitted patch, getValue-plugin.patch, failed testing.

The last submitted patch, getValue-plugin.patch, failed testing.

The last submitted patch, getValue-plugin.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 3: plugin_2620218_3.patch, failed testing.

The last submitted patch, 3: plugin_2620218_3.patch, failed testing.

The last submitted patch, 3: plugin_2620218_3.patch, failed testing.

The last submitted patch, 3: plugin_2620218_3.patch, failed testing.

The last submitted patch, 3: plugin_2620218_3.patch, failed testing.

The last submitted patch, 3: plugin_2620218_3.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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