Problem/Motivation
\Drupal\Core\Field\FieldItemBase::__get()
checks whether a typed data property with the given name exists in order to decided whether to store the value in a typed data property or the internal values array. However, it checks this by using the array of already instantiated properties, rather than checking whether a property with that name should exist through its property definitions.
Proposed resolution
Use the property definitions rather than the list of already instantiated properties.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#52 | interdiff.txt | 3.31 KB | Hardik_Patel_12 |
#52 | 2413471-52.patch | 6.78 KB | Hardik_Patel_12 |
#51 | 2413471-51.patch | 6.77 KB | andypost |
#51 | interdiff.txt | 1.64 KB | andypost |
#33 | interdiff-31-33.txt | 2.3 KB | tedbow |
Issue fork drupal-2413471
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
XanoThis is the fix only. I'll think about how to test this when I have more time.
Comment #2
XanoComment #8
tim.plunkettLooks sane. Can we get some test coverage?
Comment #9
XanoThanks for the reminder. Is there a dedicated test for
FieldItemBase
already?Comment #10
XanoBump.
Comment #13
Xano@tim.plunkett: I am trying to find out the right way to test this patch.
Comment #14
BerdirThis is by design, we explicitly avoid creating property objects to get non-computed values. This would be a considerable performance regression.
Comment #16
Xano@Berdir: Do you have other ideas on how to solve this, or should we just keep the workaround from
\Drupal\plugin\Plugin\Field\FieldType\PluginCollectionItemBase::__get()
?Comment #17
XanoTalked to @berdir on IRC. He said this works as designed.
Comment #18
XanoActually, what about this solution? This should address @berdir's performance feedback, yet allow values of computed properties to be retrieved.
Comment #22
BerdirThis might work, but we need to check what the test fail there is about.
Comment #24
tedbowThis 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.
Comment #27
tedbowForgot to update MigrateTrackerUserTest as mentioned in #24
Comment #28
Wim LeersI'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.
Comment #29
mlncn CreditAttribution: mlncn at Agaric for Teachers with GUTS commentedHere 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.
Comment #30
alexpottStill needs an explicit test.
Comment #31
tedbowas per @alexpott's request here is a test.
Comment #32
andypostwould be great to replace that with Classname:class instead of strings
Comment #33
tedbow@andypost thanks for the review
fixed
Comment #34
Wim LeersPinged @Berdir for a review. I don't feel qualified to review this.
Comment #35
BerdirI'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.
Comment #36
andypostLooks great to me, just no idea about backport to 8.2 because latest patch release is out
Comment #37
BerdirWondering 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.
Comment #38
andypost@berdir yep, sorry xposted.
I'm using this because affected by #2563981: ContentEntityBase get and __get() works differently
Comment #39
BerdirYes, 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.
Comment #41
tim.plunkettThis soft-blocks #2905922: Implementation issue for Layout Builder, but the last patch works great.
Comment #42
tedbowre @Berdir's comment in #39
@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.
Comment #43
tim.plunkettIs #2934192: FieldItemBase::getValue() returns partial or full values depending on state a dupe of this?
Comment #49
geek-merlin> 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.
Comment #50
andypostComment #51
andypostRe-roll for 8.9 and 9.1, NW is for summary
Comment #52
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedPatch failed to applied for 9.1.x , re-rolling for the same . Kindly review.
Comment #56
catchStill 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.