FieldItemBase must check property definitions instead of already instantiated properties

Looks sane. Can we get some test coverage?

This is by design, we explicitly avoid creating property objects to get non-computed values. This would be a considerable performance regression.

This might work, but we need to check what the test fail there is about.

This should fix the failing tests.

The problem was that the filter module wasn't being enabled for a couple of tests.

The new changes would cause the filter plugin to be loaded for instance in \Drupal\taxonomy\Entity\Term::getFormat().

The filter module is a dependency of text which a dependency of both node and taxonomy which were already be enabled for the affected tests. So in real site the filter module would already be enabled.

Forgot to update MigrateTrackerUserTest as mentioned in #24

I'd like to RTBC this, especially since @Berdir says in #22 that this looks good. But @tim.plunkett asked for tests in #8.

I don't know this system well enough to know what that test coverage should look like. Pinging Tim.

Here by way of #2710421: Export URL alias configuration for nodes by way of #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias... can we make sure we do tests in that particular use case, allowing REST clients to get the path alias with an entity? Here we already make sure we don't break existing functionality with the more helpful getter.

Still needs an explicit test.

as per @alexpott's request here is a test.

+++ b/core/tests/Drupal/Tests/Core/Field/FieldItemBaseTest.php
@@ -0,0 +1,72 @@
+    $data_definition = $this->prophesize('\Drupal\Core\TypedData\ComplexDataDefinitionInterface');
...
+      ->willReturn($this->prophesize('\Drupal\Core\TypedData\DataDefinitionInterface')->reveal());
...
+      '\Drupal\Core\Field\FieldItemBase',
...
+    $typed_data = $this->prophesize('\Drupal\Core\TypedData\TypedDataInterface');
...
+    $data_definition = $this->prophesize('\Drupal\Core\TypedData\ComplexDataDefinitionInterface');
...
+      '\Drupal\Core\Field\FieldItemBase',

would be great to replace that with Classname:class instead of strings

@andypost thanks for the review

fixed

Pinged @Berdir for a review. I don't feel qualified to review this.

I'll try to review this soon, but the exact use case isn't really clear anymore to me. I don't think this approach works for what Xano wanted to do initially, not if there is a value. I think we need at least an updated issue summary with the problem this solves and probably also an integration/kernel test to show how it can actually work.

Because it is *not* needed for computed fields like path, we are solving that differently now.

Looks great to me, just no idea about backport to 8.2 because latest patch release is out

Wondering if you crossposted with my comment? As I said, I'm not sure right now about the exact use case this solves and we definitely need a better issue summary and IMHO also better and "real" tests.

@berdir yep, sorry xposted.

+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -134,14 +134,21 @@ protected function writePropertyValue($property_name, $value) {
+    // Instantiate the property if it exists, and return its value. We do this
+    // last, because property instantiation is often slower than the previous
+    // retrieval methods.
+    elseif ($this->definition->getPropertyDefinition($name)) {
+      return $this->get($name)->getValue();
+    }

I'm using this because affected by #2563981: ContentEntityBase get and __get() works differently

Yes, but I this will not fix much about that, because it only gets called *if* there is no value *and* the property is not computed. But if it's not computed then I'm not sure that getValue() should be doing anything except.. returning the value.

I'd like to see how exactly that affects real use cases. It will definitely not help #2649646: Normalize path fields as part of the entity: allow REST clients to know the path alias in any way as far as I see.

This soft-blocks #2905922: Implementation issue for Layout Builder, but the last patch works great.

re @Berdir's comment in #39

I'd like to see how exactly that affects real use cases.

@tim.plunkett can you explain how this affects #2905922: Implementation issue for Layout Builder
I didn't see this issue mentioned there. It might give us a better idea of a "real" bug this is solving.

Is #2934192: FieldItemBase::getValue() returns partial or full values depending on state a dupe of this?

> Is #2934192: FieldItemBase::getValue() returns partial or full values depending on state a dupe of this?

@tim, On first sight it looks so. But this patch changes FieldItemBase::__get(), while the other changes TypedData//Map::getValue(). Also the symptoms are quite different so i don't see how one might fix the other.

Re-roll for 8.9 and 9.1, NW is for summary

Patch failed to applied for 9.1.x , re-rolling for the same . Kindly review.

Still applies but I agree with Berdir this could use a kernel test to show the actual situation that this helps, which isn't at all clear.