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.
I've extracted the various typed data API improvements out of the comment conversion issue such that it's easier to review them on their own. Original issue: #1778178: Convert comments to the new Entity Field API
Short summary of the improvements:
- The typed data manager now supports prototyping typed data objects via getPropertyInstance(). That's a rather big performance improvement compared to creating the same kind of typed data objects each time. That's used by the entity API.
- For the above improvement to work out the ContextAwareInterface has been improved and now also contains getPropertyPath() - what is inline with the usage of property paths of the symfony validator component.
- This patch changes how the EntityNG BC-compatibility mode works. Instead of having a the compatibility mode as state in the entity object it uses a decorator object. That better separates things and avoids us having to enable/disable compatibility mode each time + should help with further conversion issues as it allows us to do an interim conversion step: Migrate to the new API but do not use EntityNG yet, instead just provide an EntityNG-compatibility object from $entity->getOriginalObject(). Thus meanwhile, the regular entity would be the BC-compatibility one.
Further improvement issues:
#1867888: Improve TypedData date handling
#1867880: Bring data type plugins closer to their PHP classes
#1868004: Improve the TypedData API usage of EntityNG
Comment | File | Size | Author |
---|---|---|---|
#55 | entity-bc_decorator_fix-1869250-55.patch | 1.17 KB | plach |
#47 | d8_typed_data_follow_up.patch | 591 bytes | fago |
#35 | d8_typed_data_improvements.patch | 114.5 KB | fago |
#35 | d8_typed_data_improvements.interdiff.txt | 3.9 KB | fago |
#32 | d8_typed_data_improvements.interdiff.txt | 2.14 KB | fago |
Comments
Comment #1
fagoComment #3
fagoNext try - hopefully I've got all relevant changes this time.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commented#3: d8_typed_data_improvements.patch queued for re-testing.
Comment #7
fago#3: d8_typed_data_improvements.patch queued for re-testing.
Comment #8
fagoNow a different test fail? Both tests pass for me locally.
Comment #9
BerdirIs there reason we can't pass $values to the entity? If that doesn't work then there isn't much use in having that argument at all, is there?
Ah, saw it below, the argument expects a different format of the values. Would it make sense to convert them instead of adding them through magic setters?
Every time I see this I think we should kill field_attach_load_revision() in favor of a isDefaultRevision() check in field_attach_load()...
Should be Contains now ;)
Not sure that comment is grammatically corrt.
Not sure what the coding standards say about documentation magic methods, but probably not "Magic method" :)
@return should be above @see and have a short one line description.
This makes sense. What happens if you do something like $node->type = 'your-now-something-else';?
I guess that's not really supported anyway but if it isn't, then we should prevent it from happening. Make the bundle field read-only or so?
Is there a reason for doing it like this instead of providing temporary helper methods for that? Needs to be removed either way and this is quite the ugly :)
A search & replace gone wrong? ;)
Changed look good generally, but it's quite as long patch, containing various different things which makes it hard to review.
Comment #10
fagoI think we could pass $values if we make sure we arrange the values array with proper langcode keys. I did differently though as explicitly setting them could allow a change-tracking mechanism to keep track of it, while this change-tracking mechanism should not track the originally passed (e.g. loaded) values.
Good point - that's not supported. I don't think that's a big deal as you can still create a new object and copy the values over if you really need to. Still, we should handle the case nicely. We could mark any bundle field as read-only, although that doesn't get checked at run-time (yet). We'd also need to inherit the read-only flag then. I think that's something we should focus on in its own follow-up.
Yes. Every other solution passing on references or returning references makes the object properties of the EntityNG entity to references, what results in problems when cloning objects (and so in entity uuid test fails). I found no way to actually copy $this->values in __clone() once it became reference (thank you PHP).
Updated patch fixes the mentioned commenting issues. Also, merged and fix conflicts.
Comment #12
fagoLooks like it got broken by #1868274: Improved EntityNG isset() test coverage :(
Comment #13
fagoAttempt to fix with the following interdiff - seems to break things :/ (run all entity tests)
Comment #14
fagooh, turns out my fix seems to already work as expected - my test environment got just hit by #1871032: Taxonomy module fails to install on MyISAM due to too long schema index ;-)
Thus, uploading combined patch. Interdiff is #13.
Comment #15
fagoComment #17
fago#14: d8_typed_data_improvements.patch queued for re-testing.
Comment #19
fagoWhile the new isset() code works, it doesn't fit the bill for the current unserializing/REST implementation as newly created entities have set fields by default. Adding a hunk to unset empty fields should fix it and allow REST module to properly determine which field have been part of the data.
Comment #20
YesCT CreditAttribution: YesCT commentedInstead of unsetting the empty fields, should we fix it so they are not set empty in the first place?
Comment #21
fagoad #20: We have fields empty but set by default for better performance (we have 1 empty item that gets initialized via cloning). As the item is still empty I don't think it matters much whether it's just empty or empty and not set.
Still, we could unset empty fields during entity creation in general, but I was not able to come up with a good reason to do so, so I thought it's better to keep it where it's used. If others think different I'd happy to follow though.
Comment #22
YesCT CreditAttribution: YesCT commented#19: d8_typed_data_improvements.patch queued for re-testing.
Comment #24
YesCT CreditAttribution: YesCT commentedhere is the conflict when trying to reroll:
I gave it a try:
I was not sure about the new second half of the OR:
Comment #25
fagoThanks, YesCT - changes look good to me.
Comment #26
fagoImo this is ready, lying around for a long time as part of #1778178: Convert comments to the new Entity Field API and blocking various issues - RTBC anyone? :-)
Comment #27
das-peter CreditAttribution: das-peter commentedI did mainly a visual review, architecture wise I trust fully fago (there were also quite some discussions in IRC about a lot of this stuff) and the tests pass :)
This are mainly nitpicks, thus I'll make the mentioned changes asap.
Not sure if this collides with the coding standards: When calling class constructors with no arguments, always include parentheses. I think the return value from
$this->entityClass()
should go into a variable first.I think "ways" should be removed.
Shouldn't this pass the
$langcode
to the method of the decorated entity too?Shouldn't this pass the
$new_value
to the method of the decorated entity too?We should use
format_string()
here, right?Comment #28
das-peter CreditAttribution: das-peter commentedOkay, forget about
$this->entityClass
. I wasn't aware this is a class variable and thus already matches to the coding standards...Other changes are made in the updated patch.
Comment #29
fubhy CreditAttribution: fubhy commentedSame here... So here comes my nitpick review. Will provide a patch too afterwards.
We shouldn't have these full namespaces in the method docblocks. Should be simply "Implements EntityInterface::getBCEntitiy()." etc.
Wait... I read that before somewhere :P.
"possibly" is wrong here (should be "possible", no?).
That looks weird. I would be happier if it was a is_array($values) && empty($values) even if that's longer.
missing whitespace (the ListInterface)
I still don't understand the policy for this fully. Do we use a backslash in the @file docblock now or not? Most of core doesn't but than again there are about 100 occurrences with leading backslash.
Should be "a list item" no? I wish my english was better :P
Why? It was correct before.
Should be 'Contains'
Getting more and more confused about our policies regarding namespaces in docblocks ...
Comment #30
BerdirWe still haven't decided on exactly how we reference namespaced classes, so I would just leave that stuff alone except for the Contains/Definition of part.
Comment #31
fubhy CreditAttribution: fubhy commentedAlright, that makes it easier.
Comment #32
fagoGood catches - thanks!
That's the only one I'd disagree, to me the other one would be easier to read. Also it's already used that way in the initial if(), so if we change it we should do so in both cases.
Thus I had another look at this if/elseif/else chunk and noted it can be simplified it to an if/else. Does attached patch does so by keeping the === array() way - that way you can easily see the two values which pass the if(), NULL and array().
http://drupal.org/coding-standards/docs#namespaces says:
Thus the (current) standard is to use this initial backslash here. So I converted the remaining lines to follow this also, what makes it at least for this patch consistent ;-)
Added all improvements to the Git branch (typed_data_improvements) and re-rolled the patch.
Comment #33
klausi@var data type does not match default value, use "@var string|FALSE" instead.
That looks ugly, as you never know what getParent() returns if you don't know much about the object. Splitting that up into getListParent() and getDataParent() only makes the situation more complex, so I guess this is ok.
"the the". Also, "leave it empty" sounds weird here. Better use "Otherwise omit the parameter."? Or just don't mention that at all, the parameter is marked as optional, so people know how to use it.
Same here.
Same here. And possibly quite a few other places.
Additional test case for setting NULL values form the last patch in #1868274: Improved EntityNG isset() test coverage is missing.
Comment #34
fagoThis is not supposed to cover #1868274: Improved EntityNG isset() test coverage - it's just that the changes introduced by this patch unfortunately required cleaning up the accidentally committed code from #1868274: Improved EntityNG isset() test coverage. Still, this is not dealing with introducing #1868274: Improved EntityNG isset() test coverage - it just contains required fixes of what's there. Anything more than fixing existing stuff can be done in #1868274: Improved EntityNG isset() test coverage.
Comment #35
fagoComment #36
klausiOk, I do not feel fully qualified to RTBC this, but since we already had some other positive feedback and even the nitpicking comes to an end for this large patch ==> good to go.
Comment #37
sunI looked over this patch and did not find anything major that would or should block a commit.
The only more general issue I had while reviewing is that a lot of the new (and existing) entity data handling methods are performing very complex operations, but they do not contain any inline code comments that would explain what is being done and more importantly why. Therefore, I stared on a couple of code blocks and tried to mentally decipher what the code is doing, and why, and whether the code is correctly doing what it attempts to do, but in almost all cases, I ended up with "Hrm... whatever, I hope it's ok."
The only other and partially more elaborate comments are actually @todo comments... and most of those are even harder to decipher for someone who's not neck-deep into the code ;)
Could we create a separate, dedicated issue for improving the technical inline code documentation? And/or alternatively, can we make sure to vastly improve the docs through other issues/patches? :)
In any case, I think this is RTBC and we can move forward with this patch.
Config\Entity only overrides selective Entity\Entity methods thus far - these methods appear to be identical and not overridden in any way; was there any reason to duplicate them?
We can remove them in a follow-up patch though - no reason to hold up the commit for that.
This slightly clashes with our current notion of $entity->original during saving/updating, no?
However, if I get this right, then this BC layer is supposed to be removed anyway before release, so I guess the name clash doesn't matter for now.
That said, do we have a "Plan B" in case we do not manage to kill the BC layer before release? Should we perhaps create an issue for such a plan B, tagged with "revisit before release"?
What I do not really understand in the new EntityNG design is how default values of entity properties are handled — e.g., as in this particular case, $this->langcode actually has a default value, which is simply removed by init() and thus entirely lost, no? So unless I'm missing something big, we're losing an essential and required concept of classes.
I guess this question is not really related to this issue/patch though... Do we have an issue for that already?
Thanks!
Comment #38
YesCT CreditAttribution: YesCT commentedFollow-ups from #37
Comment #39
BerdirYes, this needs to go in. It is blocking multiple major/critical issues, so setting to major as well.
Comment #40
fagoThanks for the review sun!
hm, not good. Yep, let's improve inline comments in #1877632: Improve comments and clean-up code for EntityNG and the TypedData API. I can go through the code and add more comment but I obviously I could use hints what parts in particular are confusing as I'm missing the outside-view. Please post hints at #1877632: Improve comments and clean-up code for EntityNG and the TypedData API.
Right. We could still change this to another name to avoid confusions though, any suggestions? It's supposed to removes any possible (backward compatibility) decorator in use.
Nop, we don't have a plan B and imho we really shouldn't but focus on moving on. Having both in place is not only a DX nightmare, anything using BC is also slower. Let's make sure we convert everything! Issue: #1818580: [Meta] Convert all entity types to the new Entity Field API
I posted some thoughts on how we could speed up EntityNG adoption there: http://drupal.org/node/1818580#comment-6893030.
True, they are needless. In my thinking Config\Entity did not extend Entity\Entity (I guess it was that way some time ago?) - anyway we can remove them now.
Yes, we are loosing that. That's unavoidable with fields as objects :/
#1777956: Provide a way to define default values for entity fields - imho it should suffice to create a method that provides the default value and incorporate it via entity_create(). The method should probably read the default value from the definition by default such that defining a simple, static default does not require a custom class. But let's discuss this over there.
@YesCT: Thanks for creating follow-up. Yep, let's get this in asap and care about the noted issues there.
Comment #41
catchThanks for splitting this into its own issue. Should make the comment conversion much easier to review. Committed/pushed to 8.x.
Comment #42
fagoThanks catch, posted a patch for #1877638: Rename getOriginalEntity() and remove duplicated functions from ConfigEntityBase.
Comment #43
tstoecklerI'm sorry if this has been discussed above (couldn't find anything) or if I'm missing something, but I'm pretty sure this should forward
$operation
as well, not just$account
.Re-opening for that. Should be a quick fix. Don't know if we need tests, though.
Comment #44
tim.plunkettAnd in the decorated method:
return $this->__call(__FUNCTION__, func_get_args());
Comment #45
webchickComment #46
plachSorry if this has already been explained, but why do we override the new Entity methods in ConfigEntityBase and provide the same implementation?
Comment #47
fagoad #46: see #1877638: Rename getOriginalEntity() and remove duplicated functions from ConfigEntityBase
ad #44: thanks, yeah we could use call_user_func there, but the BC-mode is already extra-weight enough and potentially used quite a bit. So I think having direct method calls is good here.
ad #43: Good catch - this got changed and obviously not fully updated here. Follow-up patch attached.
Comment #48
das-peter CreditAttribution: das-peter commentedI say RTBC ;)
Comment #49
sunI only wanted to create a review for #1877632: Improve comments and clean-up code for EntityNG and the TypedData API, but while cycling through the patch once more, I had a couple of further questions, so I'm posting the review here instead of over there.
Some of them are related to this issue/patch, some others are more generic issues with typed data.
1) What kind of additional optional definitions are added here, and why do they overlap with the bundle definitions? Why are additional values from an optional definition for a bundle key left out and not merged in (due to +=)?
2) $constaints['bundle'] == $bundle, so parts could be simplified here.
The comment can be removed. Or perhaps replaced in order to explain why $this->bundleKey is re-evaluated here - isn't exactly this negotiation happening in __construct() of a storage controller already? I.e., isn't $this->bundleKey == $bundle anyway already?
Why are the passed in values to create() set directly as property $name, whereas all other code is using $name->value?
It's not clear why the value assignment does not account for multiple value records.
Can we replace "the Drupal 7 way of..." with its actual meaning? And also clarify what the difference to the non-decorated way is?
It's not clear to me what this comment tries to communicate and educate about. The (logical/linguistic) context and topic is missing. It also sounds/feels like the comment rather belongs onto a method's phpDoc block instead of the class level (though not sure).
Who or what is setting $this->decorated->fields[$name] (the corresponding
__set()
method does not), and why can this code unset that property without re-instantiating it?I don't really understand why this is copying values from one language to another, whereas there are actually no values for the other language?
$definition seems to be unused? Do we need a hasProperty() method?
Why do we unset to an array instead of NULL?
An entity type with the name "number" or any other typed data type name will clash... Shouldn't we prefix these dynamic entity type data types with "entity_" or something?
Hm. create() was a lot more clear as a method name here.
It looks like we're copying the entire property definitions into every entity object? Did we perform benchmarks on memory consumption?
"entity type" with a space instead of a underscore ("entity_type") was a bit unexpected here - also not sure why the arguments are passed as an array instead of two arguments?
Hm. I've to admit it looks like I missed a couple of issues and commits regarding the TypedData API.... Thus, no idea what a "strict mode" is. But regardless of that, the DX of
setContext()
appears to be awkward and could really use some love.Why are empty translations filtered out?
I think I should be able to create a translation that is empty? E.g., when I purposively want to have an empty translation?
I don't really understand why all of this direct munging with values happens in the Entity class instead of the BC decorator?
1) It really looks like we could use a
hasProperty($name)
method, since all of these conditional checks essentially pass an array around, while a simple Boolean TRUE/FALSE is all that's needed.2) Is there no
isset()
method on properties? The comparison against NULL and also unsetting by setting NULL does not look good to me.Isn't this exactly the same code as
Entity::label()
, except for a final:1) Typo in "$langocde" ($langcode).
2) Why is the langcode trimmed to 2 characters? That's a pretty bold assumption...?
3) The @todo could be clarified; had to read it twice to understand what it talks about, which is partially caused by missing context. E.g., you could do:
// Determine the language code of this translation.
// @todo Add proper methods to get and also change the langcode.
"usually needed" by what and why?
According to this revised implementation,
getValue()
will return NULL if an item is empty, so why do we check forisEmpty()
first?Is there any reason for why we allow $this->list to be NULL at any time?
would ensure that $this->list is always an array, which in turn would simplify a lot of the other methods that currently need to account for a potential NULL value - even though their behavior on NULL is identical to the behavior of an empty array.
Some code is accessing $this->list directly, while some other code is using the $this->list() method - the majority of code accesses the property directly instead of using the method, so I think we want to change the instances that call into the method.
Most foreach loops call the list values $item, while __clone() calls it $property.
Wouldn't a star ('*') be more appropriate? 0 will prevent an individual list item to have a diverging data type, no?
E.g., if someone were to write a entity_reference_ng module in contrib that allows to reference multiple different entity types, then we'd have a list of varying data types, or am I mistaken?
This loop and value assignment seems to diverge from the regular entity storage controllers? (i.e., this is the same controller method I mentioned at the beginning, but this one does not omit [0]['value'])
Comment #50
fagoThanks for the great review sun, but can we move this to a new issue? Else, it becomes hard to keep the overview and properly track the small follow-up patch from #47.
Thus, responded at #1877632-7: Improve comments and clean-up code for EntityNG and the TypedData API
Comment #51
webchickHm. Seems like tests should've barfed about #47, so we should add test coverage IMO. Could we open a follow-up for that patch so we can mark this one fixed?
Comment #52
webchickComment #53
fagoYes, the BC decorator does not have full test coverage. Mostly, it's tested indirectly via existing tests that keep working only thanks to it, e.g. all tests requiring field API. But method implementations on the BC-entity that are for *new* interfaces will never be used on the BC-entity, thus are in-fact useless (while they are needed to fulfill the new interfaces).
So, as those methods are a) not used and b) part of the only temporary existing BC mode I think it would be a waste of effort to write tests for that.
As #47 should be ready and has been RTBCed here already, could we just move on with it here?
Comment #54
webchickOk, fair enough.
Committed and pushed #47 to 8.x.
Comment #55
plachI've another small follow-up to propose, stemming from #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget. Here is a patch. We can commit this here or as part of the other issue, as you prefer.
The problem is very simple: if you try to access
$entity->original
on a BC entity you get a fatal because the original entity object is treated as an array in the magic getter. The attached patch adds a check to detect this situation.Comment #56
tstoecklerMarking "needs review" for people to pick this up.
Comment #57
fago@webchick: Thanks!
ad #55: webchick already requested to do follow-up fixes as separate issues, so could we move this to its own issue? Let's just create follow-up issues for everything left and mention them here.
Comment #58
yched CreditAttribution: yched commentedTiny and trivial followup : #1880922: Remnants of entity_view_prepared flag
Comment #59
plach#55 moved to #1880926: Fatal error when retrieving $entity->original on a BC entity.
Comment #60
fagoThanks, responded there.
Comment #61.0
(not verified) CreditAttribution: commentedUpdated issue summary.