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.
It is common to provide getters and setters for the fields on our entities.
The EntityChangedTrait already handles the methods for the EntityChangedInterface.
There was a method in the entity that was not defined in any other interface.
Comment | File | Size | Author |
---|---|---|---|
#6 | interdiff.txt | 3.85 KB | Mile23 |
#6 | 2898376_6.patch | 623 bytes | Mile23 |
| |||
#2 | 0001-Clean-up-interface-implementation.patch | 4.37 KB | vegantriathlete |
|
Comments
Comment #2
vegantriathleteI'm just trying to get some stuff done while I'm here at DrupalCamp Colorado. Please forgive me for not naming the patch according to the typical convention.
Comment #3
vegantriathleteI thought I recalled something in the documentation about providing getters and setters, but was not able to locate it quickly. It would be good to provide an @see in the interface referencing that documentation (if it does actually exist).
Comment #4
eojthebraveLooks like the documentation you're referring to might be what is on this page https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... - it's just a single line. So may or may not be worth linking to.
The removal of this method isn't exactly related to this issue. But it also appears that it's safe to do because it currently overrides the same method from \Drupal\Core\Entity\EntityChangedTrait, which has the exact same code.
Seems like there might be an extra comment here?
Should this also update \Drupal\content_entity_example\Entity\Controller\ContactListBuilder::buildRow to use these new getter methods?
Comment #5
Mile23Interfaces define the public API of the service, as a promise to the code that uses the implementation.
Since that's the case, they also define the code that won't change. Other methods on the object might change. This implies that if you make an interface with getters and setters, you're locking yourself into maintaining those getters and setters. It also implies that the pieces of information that the object provides will always include whatever the getters give you. In our case this isn't such a big deal, but in this sense it's not a best practice to have getters and setters.
There's a whole debate about whether getters and setters are considered good. If the interface is meant to give you a specific piece of information (like
getName()
would), then you should add it. But if the purpose of the interface is to perform a task, then implementation details should be hidden.So with that in mind, here's my review of the patch in #2:
This patch adds getters and setters to the interface and then implements them. But they are not used anywhere else in the module. So basically we're adding some methods and extra lines of code we don't need.
The only place we might use the interface is
ContactListBuilder::buildRow()
:Here we see that we might only need getters for first name, gender, and role. So then we ask: Do we really need getters for these three things, and do we need them in the interface?
ContactInterface
exists so that we can combine the other interfaces and make promises aboutContact
as belonging to the content entity API.Contact
subclassesContentEntityBase
, which defines how it works. If you look atContentEntityBase::__construct()
you see that entities don't want or need getters or setters, by design. That allows them to be highly arbitrary when it comes to existing fields. The entity is constructed out of a blob of data which is then mapped to the properties based on the entity definition.So do we extend our entity implementation to have convenience methods? We do that for
getCreatedTime()
, which the patch rightly removes. :-) We also overrideEntityChangedTrait::getChangedTime()
which we should probably also remove.I don't think we need to add getters and setters here, particularly to the interface, because that's not the 'spirit' of the entity API. And also because it only adds more lines of code to need to change if we were to change our field definitions. We're planning on removing the gender field, which will be more effort if we add a gender getter to the interface: #2898380: Make content entity example more gender-inclusive
So: As cleanup, remove
getCreatedTime()
andgetChangedTime()
fromContact
.Thanks.
Comment #6
Mile23Made the changes from #5. Also this isn't a bug since everything works, we just have some questions about what our interface should be.
Comment #8
Mile23And we got it. Thanks!