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
Part of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
See the detailed explanations there and look at the issues that already have patches or were commited.
Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Reroll the patch if it no longer applies. | Instructions | ||
Add automated tests | Small tests to make sure setters work properly. Instructions | ||
Draft a change record for the API changes | Instructions | ||
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
User interface changes
NONE
API changes
EntityDisplayModeInterface
now uses setTargetEntityTypeId()
and getTargetEntityTypeId()
to get and set its $targetEntityType
property.
The EntityDisplayModeBase
class implements the EntityDisplayModeInterface
class, and these changes are reflected there too.
Comment | File | Size | Author |
---|---|---|---|
#67 | 2030613_67.patch | 5.67 KB | Mile23 |
#67 | interdiff_65.txt | 4.73 KB | Mile23 |
#65 | 2030613_65_protected_properties_only.patch | 1.2 KB | Mile23 |
#63 | 2030613_63.patch | 7.39 KB | Mile23 |
#59 | interdiff_57.txt | 1.35 KB | Mile23 |
Comments
Comment #1
plopescPart of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
See the detailed explanations there and look at the issues that already have patches or were commited.
Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())
Comment #2
hernani CreditAttribution: hernani commentedWorking in Prague Sprint
Comment #3
hernani CreditAttribution: hernani commentedComment #4
balagan CreditAttribution: balagan commentedComment #5
balagan CreditAttribution: balagan commentedI am not sure if there is anything to do. The properties can be found here.
I have found the following getter and setter methods up in the hierarchy tree:
EntityDisplayModeBase class
getters:
->getTargetType()
ConfigEntityBase class
getters:
->getOriginalId()
->isSyncing()
->status()
setters:
->setOriginalId()
->setSyncing()
->setStatus()
Entity class
getters:
->entityType()
->routeProvider()
->label()
setters:
->enforceIsNew()
Maybe a setLabel(), setTargetType(), setRouteProvider(), setEntityType() method is meaningful, although I am not sure about especially the two latter.
Comment #6
balagan CreditAttribution: balagan commentedEnclosed a patch with setLabel() and setTargetType(), although I am not sure about how to name them. See this comment. I have also changed the properties to protected.
Comment #7
balagan CreditAttribution: balagan commentedEnclosed a patch with the same changes as in the previous patch, but properties remain public.
Comment #8
balagan CreditAttribution: balagan commentedComment #9
balagan CreditAttribution: balagan commentedIn previous patch I forgot to return a value, fixed that. Enclosing patch with protected properties.
Comment #10
balagan CreditAttribution: balagan commentedSame patch (setLabel return $this), properties stay public.
Comment #14
Berdir10: 2030613-EntityViewMode-4.patch queued for re-testing.
Comment #15
balagan CreditAttribution: balagan commentedComment #16
fagoWe should move all method to the parent base class, where the properties are defined.
ONe comma too much.
Missing a new line.
Should be string $target_type - also the parameter name needs to be changed.
No full sentence (misses trailing point).
return $this.
So let's rename the getter and the setter to go with the "getTargetEntityTypeId" naming, to be consistent with FieldConfig and inline with the entity property name.
no camel casing here.
should be return $this
Comment #17
balagan CreditAttribution: balagan commentedComment #18
balagan CreditAttribution: balagan commentedTried to implement the changes recommended by fago in this patch.
Comment #19
balagan CreditAttribution: balagan commentedComment #21
balagan CreditAttribution: balagan commentedThe test failed, I have tried to fix the call to undefined method problems in this patch. Anyone is welcome to commend on the other errors warnings, here: https://qa.drupal.org/pifr/test/755168
I don't really get anything else except the mentioned undefined methods.
Comment #22
balagan CreditAttribution: balagan commentedComment #24
balagan CreditAttribution: balagan commentedTrying again.
Comment #25
balagan CreditAttribution: balagan commentedComment #26
fagoCode style
Parameter should be target type id.
return $this
display mode.
This needs an issue summary mentioning the API changes of the target entity type method (use the issue summary template).
Comment #27
fagoComment #28
balagan CreditAttribution: balagan commentedTrying again. Also changed the parameter to target_type_id in the other file.
Comment #29
balagan CreditAttribution: balagan commentedComment #30
fagoAs discussed, it should be getTargetEntityTypeId() to be consisitent with FieldConfig. Everywhere here + the setter.
The entity type to be displayed.
Comment #31
balagan CreditAttribution: balagan commentedTrying to fix the errors.
I am whether name should be mentioned in the comments when the function is called getTargetEntityTypeId, and supposedly returns an id.
Comment #32
balagan CreditAttribution: balagan commentedComment #34
balagan CreditAttribution: balagan commentedMissed some methodnames in the previous patch. This patch passed tests on my localhost.
Comment #35
balagan CreditAttribution: balagan commentedComment #36
fagoLooks good, thanks. This should include a small test to make sure the newly added setter works though. Besides that we need an issue summary that lists the API change of the target entity type getter.
Comment #37
balagan CreditAttribution: balagan commentedComment #38
balagan CreditAttribution: balagan commentedI have updated the issue summary with the API changes. If that is acceptable, and only the writing of the test is needed, then I think the novice tag should be removed from this issue.
Comment #39
Xano34: 2030613-EntityViewMode-34.patch queued for re-testing.
Comment #40
balagan CreditAttribution: balagan commentedI have set the issue to needs work, because @fago said in comment #36 that some tests are needed to make sure setters are working properly.
Comment #41
balagan CreditAttribution: balagan commentedComment #42
XanoComment #43
alexpottPSR-4'd, made properties protected and added tests.
Comment #45
alexpottFixed tests
Comment #46
scottrigbyUpdated issue summary & working on reroll.
Comment #47
scottrigbyComment #48
scottrigbyComment #49
scottrigbyApplied with
curl 2030613.45.patch | git apply --3
.Attaching the reroll with no changes.
Also, #2030613: Expand EntityViewMode (really EntityDisplayModeBase) with methods was since committed, so will add a new patch with interdiff to remove that next.
Comment #50
scottrigbyOnly removed TODO (interdiff attached).
Since I rerolled this, someone else should review.
Comment #51
scottrigbyBTW the patch looked perfect to me while reviewing. I didn't test it, because in reviewing the automated tests added did the exact same steps I would have gone through, and they pass. I didn't mark RTBC only because I rerolled.
Comment #52
MKorostoff CreditAttribution: MKorostoff commentedConfirming that the re-roll in #50 applies cleanly
Comment #53
alexpottthe PHPstorm file shouldn't be part of the patch
Comment #54
scottrigby:facepalm: IntelliJ file removed!
Comment #57
Mile23Reroll of #54, with 3-way merge happiness.
Comment #59
Mile23Unit tests were mocking a class that had been moved.
Comment #61
alexpottThe changes to these files are not this patch afaics
Comment #62
Mile23Yah, I literally pulled 8.0.x, made this patch, and before I uploaded it the files moved. Will reroll.
Comment #63
Mile23EZ re-roll.
Comment #64
alexpottThis is an API change and, whilst it is better, it is not so much better that we should break our API change policy. So let's rename
setTargetEntityTypeId
tosetTargetType
. If you feel strongly that this is confusing then we should deprecategetTargetType
and add the newgetTargetEntityTypeId()
andsetTargetEntityTypeId
methods.There are no usages outside of tests so let's not add this.
EDIT: For clarity
Comment #65
Mile23OK, so a couple of things here:
1) The reroll includes all the work done around
setTargetEntityTypeId()
, which is apparently @Fago-approved (#30), and which @alexpott worked on in #45. :-) But I get that it might be an API change during the alpha cycle, so maybe that's a reason to hold back on it.2) I was told at a sprint and in IRC by @alexpott and @yesct that this issues and it's siblings from #2016679: Expand Entity Type interfaces to provide methods, protect the properties had a scope change to simply make all properties protected and add getters and setters as needed.
So with #2 in mind, here's a patch to
EntityDisplayModeBase
which only marks those properties as protected, and then calls it a day. Existing tests should fail if this accessibility change means anything to existing code, and obviously setting them protected signals developers not to use them moving forward.I couldn't get local tests to fail, so let's see what the testbot says.
If this strategy is a bad one, we can get back to messing around with the patch in #63.
Comment #67
Mile23Expanding on the patch in #65, and test results, here's what I did:
setTargetType()
toEntityDisplayModeInterface
. This mirrors the existinggetTargetType()
.EntityDisplayModeInterface
is only implemented by two other classes, and neither overrides any relevant methods fromEntityDisplayModeBase
.core/modules/field_ui/src/Form/EntityDisplayModeAddForm.php
andcore/modules/field_ui/src/Form/EntityFormModeAddForm.php
.Comment #68
Mile23Comment #69
daffie CreditAttribution: daffie commentedThe variables $id and $label do not have to be declared here. They are inherited from the Entity class.
Should we not use the set function for this.
Comment #70
daffie CreditAttribution: daffie commentedComment #71
Mile23Actually,
$id
and$label
are not provided by Entity. Entity has::id()
and::label()
methods which bend over backwards to avoid storing their information at that level.Also, since
$targetEntityType
is provided by this class, it makes more sense to just set it, unless there are some undocumented side-effects provided byset()
that we need. Looking atset()
, we see that it might need to set the value in a plugin collection.I'm not sure if this base class is supposed to care about that.
Comment #72
daffie CreditAttribution: daffie commented@Mile23: Yes, you are right. My mistake. The variables $id and $label must be declared.
Some variables do use the set(). And others do not. I have no idea what to do.
Comment #74
daffie CreditAttribution: daffie commentedI have reviewed the patch and it all looks good to me.
The patch is a month old so to be sure I have it retested.
The class variables are protected and the function setTargetType() is added.
There are PHPUnit tests added.
The documentation is also in order.
If retesting gives a green result I shall give the issue a RTBC.
Comment #75
daffie CreditAttribution: daffie commentedThe re-test gives all green. So RTBC.
Comment #76
alexpottWhilst I worked on one version of this patch @Mile23 rewrote this patch in #65 so I think that me committing this is fine. This issue is a prioritized change (see #2016679: Expand Entity Type interfaces to provide methods, protect the properties) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 3827a48 and pushed to 8.0.x. Thanks!