Expand Entity Type interfaces to provide methods, protect the properties

Updated issue summary.

Are we replacing public property access by getters and setters because it's cumbersome, or because property access is tight coupling and we don't want that? In case of the latter, we will also need to convert entity_create() calls to no longer provide a second argument, and we can only allow ourselves to access properties from within the entity class itself.

Woah, I didn't know this issue :) Changing title to *methods*, it's not just getters.

Contact Message and File are NG and are done, the NG conversion patch already added an interface.

User is in #2026347: Adding methods to UserInterface/AccountInterface. Will update the summary asap.

@Xano: The reason we remove the public properties for NG entities is that they're not actually public, they're just a hack to get autocomplete when we type hint on the class. There's the crazy init() method that then unset()'s them, so that __get() is invoked. Which we stop doing (type hint on classes), so that hack is useless.

That said, the methods are for convenience. It's still valid to pass values to entity_create() and access them directly. NG exposes it's fields dynamically and you can generically access them, if you want to work with entities in a generic way. That's the whole point of it's existence. Rest, Rules, edit.module, translation management, ... If you specifically access nodes and type hint on NodeInterface, it's recommended to use the methods.

updated hints to patch producers encorporating info from #5

Updated issue summary.

Updated change notice a bit.

I think the separation should be database vs config entities, not NG vs. not NG.

Also note that

For NG entities, for example Node,
all of the public properties are deleted, and the init() method is deleted.

The second line is not an API *change*, as accessing it will still work, the public properties are just a hack for autocomplete, as described above. Access already goes through __get().

While..

For non-NG entities, for example Action,
the public properties should become protected.

the second line here is an API *change*, $action->something will no longer work. So we can still add the interface methods any time we want, but changing the properties to protected is a different thing.

Fix markup for File

Updated issue summary.

Updated issue summary.

Ah, instead of creating the issues myself, I'm just going to tag this as Novice directly :)

So if all issues for the NG entities already have issues, feel free to create additional ones for the second group of entities.

Updated issue summary.

Added correct link to Expand ItemInterface (aggregator.module) with methods

Creating new class follow-up issues

Note:

* Do not add getter methods for things that already exist on EntityInterface, do not duplicate:
** id() for the primary id of the entity (no getId())
** getRevisionId() for the revision id (only relevant for entities that have revisions: node/custom block)
** bundle() for the bundle (e.g. the type for nodes, vocabulary for terms, ...)
** uuid()
** language()
* Only add setters if it makes sense conceptually: setId() is not necessary for database/content entities, as the ID is automatically generated for them when they are saved. Another example: setUUID(), this is set automatically initially and must not be changed afterwards.
* Look out for conflicts: EntityInterface for example already has isNew().

When in doubt, look at some other issues of similar entity types and discuss with a person nearby :)

Adding new follow-up issues

So… #9 suggests that this should not happen to config entities. Yet #2030605: Expand Editor with methods exists, while Editor is a config entity. Can somebody please clarify what is going on here?

I agree that

->something->value

is cumbersome, but that's not actually what happens for config entities (or at least not for Editor).

#9 is just saying lets not make them protected.
We should still add methods and never use the properties directly.

For purposes of the patch it would be helpful to make them all protected to make sure the patch works, then switch them back to public before RTBC.

Yes, what @tim.plunkett said, we type hint on interfaces, that means you don't see the public properties of the class and we need methods :)

organizing

#14 & #15: okay. I've done this at #2030605-2: Expand Editor with methods now. But… I still fail to see *why* we need to do this in the first place for config entities. The config system requires that all properties are public, so nothing prevents developers from using the public properties rather than the accessor methods. So now you have two ways to do the exact same thing. Which is inferior to what we have now.

So: why?

The config system requires that all properties are public

What about \Drupal\Core\Entity\EntityInterface::getExportProperties()? That, in combination with entity type-specific protected properties, works fine.

The config system requires that all properties are public

Yeah that's 100% wrong. The only properties required to be public are $id and $uuid, and that's for stupid reasons anyway.
See all of the properties on \Drupal\views\Plugin\Core\Entity\View, and the getExportProperties() at the bottom.

See also #1301106: Rely on methods to access entity properties [policy, no patch]

Quick question, should patches for this meta issue have tests? (I asked the same question in #2030641: Expand FilterFormat with methods)

It looks like some do and some don't.

For example, the following patches do not have tests:

- #2015123: Expand NodeInterface to provide getters and setters
- #2026347: Adding methods to UserInterface/AccountInterface

And the following do have patches:

- #1983548: Convert contact message entities to the new Entity Field API
- #1818568: Convert files to the new Entity Field API

Yes, we should have full code coverage.

added @ to some issues

Can someone please confirm if getters and setters need to be protected and change visibility into public before RTBC as stated here - https://drupal.org/node/2016679#comment-7616179

I'm going to go for setting the parameters as protected for ConfigEntityBase Entities as @tim.plunkett suggests in #18. This requires an overridden version of \Drupal\Core\Config\Entity\ConfigEntityBase::getExportProperties() function on the Entity copying the pattern in the view Entity.

Note the issue description describes this.

I did some work with Breakpoint #2030585: Expand Breakpoint with methods and wrote some tests. And I was thinking should we have some common test file to making sure that the different entity classes does not define methods mention in #12. So a simple "if function exist" test for every entity? This should be possible to write in PHPUnit and should go fast.

Updated issue summary.

Why didn't we make all of the new setter methods chainable?

$entity = entity_create('foo')
  ->setLabel('Bar')
  ->setBody('Beer')
  ->save();

Further diving into the example snippet of #25:

->setLabel('Bar')

#2015123: Expand NodeInterface to provide getters and setters added:

interface NodeInterface extends ContentEntityInterface, EntityChangedInterface {
...
  public function getTitle();
  public function setTitle($title);

However, getTitle() duplicates EntityInterface::label().

In turn, there are three Node entity methods now:

$label = $node->label();
$title = $node->getTitle();
var_dump($label == $title);
// TRUE

$node->setTitle('foo');
// Does not exist, even though "label" is the standard terminology for all entities.
//$node->setLabel('foo');

Since the whole point of this effort is DX, it would be good for DX to ensure consistency and sanity.

Cleaning up sub-tasks...

I suppose this needs to be a beta blocker (either to finish the open patches, or to roll back the ones committed so far). It makes no sense to ship Drupal 8 with half the entities having raw properties and the other half having accessor methods; that's a recipe for total confusion.

The issue summary could do with a much better description of why we would spear-head such a massive API change post-API freeze ("->getSomething() would be nicer." doesn't quite cut it. ;)). We're 7 months into it and not half done yet. Not looking good.

I don't have time right now to update the issue summary in detail, but if someone wants to work on it, here are some reasons and explanations.

We decided to use interfaces for entity objects whenever possible and type hint against those. Interfaces can not define class properties (public or not), only methods.

Assuming you're using an IDE with method autocomplete (PhpStorm, Netbeans, .. and if you're not, you should), go to a method that has a "NodeInterface $node" argument, write $node-> and you will see suggestions like getTitle(), isPublished() and so on.

Now do the same for TermInterface $term, and all you get is the generic methods from ContentEntityInterface and upwards. You don't see $term->name for example, because that's defined on Term, not TermInterface.

So, without specific methods, those interfaces are not half as useful as they could be.

Now, for content entities, everything is a field. You would have to write $node->title->value, not $node->title to access the title. And not doing so can result in pretty weird errors if you pass that to a function that expects a string or so.

Additionally, content entities do not actually work with public properties anymore, @fago came up with this crazy trick to define them and then in __construct() and __wakeup(), call $this->init(), which does an unset($this->name) and so on. The only reason to do that was to be able to get autocomplete for $term->name, which no longer works, as demonstrated above. That's why we want to remove it in #2095919: Kill ContentEntityBase::init() . And the idea was to wait on the remaining content issues (feed, comment, term) which replace those public properties with methods. We could alternatively just drop the remaining properties there, being able to remove that trick/method is my main motivation for doing this at the moment.

That said, at least for content entities, this is not an API change. $node->title->value still works. And there are no methods for configurable fields, there is no $node->getBody(), only $node->body->value. So there will always be a certain amount of inconsistencies, but that's IMHO not a reason to not do this at all.

For config entities, it's not an API change as long as we leave the public properties and don't change them to protected. Which I think we shouldn't do at this point. They don't have the additional problems that content entities have, so are IMHO not as important.

I also don't see why the inconsistency between different entity objects is such a problem. All those config entities are already way more unified than they were in 7.x, with a common API to load and save them. Moving from accessing properties to methods defined on interfaces and from functions to classes is an ongoing effort everywhere in 8.x, we're just trying to catch up with everyone else here.

So in my opinion, this is not a beta blocker. The only thing that I could possibly see as a beta blocker is the mentioned kill init() issue, because it's going to be very confusing if people pick the wrong entity type as an example, one that still relies on it. And we can do that without the three remaining content entity issues, if we have to, as described.

Aha, this is one of the "missing Entity DX beta blockers" that I keep talking about. It's at least a beta target; it has significant enough impact on DX that we should try to finish beforehand.

Edit: I'm open to discuss it being a major beta target instead, but I'd really like to see the "expected" Entity API that will be used all over modules everywhere available by beta.

Discussed with @catch. Let's do this.

These are API additions for the most part, not BC breaks unless you are subclassing Node creating your own special flower MyNode implements NodeInterface or something, I think. Let's tag the child issues "API addition", "DX", and "beta target" accordingly.

Also, catch pointed out that we should stop doing these if they are not done by RC (maybe we could add them in 8.1.0 but not 8.0.1) but that they can probably keep happening after beta. Just it would be ideal if we got as much possible in before then for contrib DX.

So the only way this would be a BC break is the following (unless I missed something):

1. You are completely implementing a replacement Node class from scratch, implementing NodeInterface.

2. You are subclassing Node, and the implementations on the Node class aren't compatible with your implementation (because if they were compatible, then even this is not an API change).

Both #1 and #2 have minimal effect, and if a contrib module was affected, it's a very simple change to make.

For code interacting with the interface, rather than implementing it, it's only an API addition.

Right, it's technically only an API addition from a procedural point of view. But from a DX point of view, if half of the entities have accessor methods and the other half don't, it's going to create a very confusing situation, which is not what we want when we're trying to "complete" APIs.

I'm comfortable with major + beta target. Thanks to Berdir for the expanded reasoning in #29.

In case of content entities and in combination with #2095919: Kill ContentEntityBase::init() , what could happen is that you take Term (Node is already changed), as an example, copy it for your own new entity type, adjust the public properties and change your init() implementation and then we remove the call to that method and very interesting things (tm) will happen. But that is also easy to fix by removing the properties and the method (there is no requirement to provide methods).

Yeah, #2095919: Kill ContentEntityBase::init() makes sense even as beta blocker to me as it might confuse people.

But from a DX point of view, if half of the entities have accessor methods and the other half don't, it's going to create a very confusing situation, which is not what we want when we're trying to "complete" APIs.

True, but we won't have all of core following best practices by beta. So if some entities use less methods than others, that's not ideal but not a big deal imho - as long as there is consistent usage of patterns for content & config entities.

Removed #2030599: Expand Display with methods from the list, because it has been removed from core to its own module.

Moved issues #2028035: Expand CustomBlockInterface with methods and #2030643: Expand ImageStyle with methods to the completed section.

This makes perfect sense to me, it's basic encapsulation which, is good here because it hides the detail from the user that you actually have to access the "value" property to get the "value".

I was trying to think about it from a new comers point of view and how I would go about it presuming i'd come straight from D7. I think some code might illustrate it better, (Note, I used node because it was simple even though it has getters now).

<?php
$node = node_load(1);
dpm($node); // Drupal\node\Entity\Node

echo $node->title; // empty, no longer works but doesn't error either.

dpm(get_class_methods($node)); // Did they add a getter?

// Nope, let's look at that property again
dpm($node->title); // Drupal\node\NodeTitleItemList

// Ok let's see what methods its got.
dpm(get_class_methods($node->title));

// The only method that looks like what we wanted is getValue().
dpm( $node->title->getValue()); // This gives us an array... hmm

// Maybe it's properties will help.
dpm(get_object_vars($node->title)); // Nope, that's empty.
?>

I'm not entirely sure how you'd figure out that you had to do $node->title->value.

EDIT: I can't seem to get any white space into my code sample...

Just to re-complete the previously raised considerations:

#26 still applies today and presents a major inconsistency both in terms of API and DX:

1. $node->title->value == $node->getTitle() != $entity->label()
2. $node->setTitle() == ?

(1) is not even fully true anymore today, in case you consider the use-case of rendering a node's title (label) for theming purposes, because title is a field now.

Next to completing the big picture of entity type specific interface methods (good idea), we should also ensure consistency between content and config entities.

Ideally, getLabel() and setLabel(). Let's make at least the shared base entity properties consistent for all entities?

Speaking of getLabel(), let's bear in mind that our current id(), uuid(), label(), etc methods are lacking a "get" prefix, which brutally stems from D7/PHP4-era thinking. In OO, all methods without a "get", "set", "has", or "is" prefix are supposed to do something beyond a trivial operation.

Despite following common OO practices, D8 requires you to learn that a seemingly arbitrary method foo() actually just returns something crucial.

In terms of consistency, industry standards, and learnability, I could not agree more; all of these (API) changes should be performed prior to releasing Drupal 8.0.

Thanks so much for the clarifications. I'll ensure #2030605: Expand Editor with methods happens.

In the issue summery is a difference between NG and non-NG entities.
- Can somebody explain what NG stands for?
- Is the difference between NG and non-NG still relevant?
- If it is not relevant any more can I remove it from the issue summery?

This issue has been given a priority for major and tagged with "beta target" and "rc deadline".
- Does that mean that related sub-issues are also priority major and tagged with "beta target" and "rc deadline"?

NG stood for Next Generation which (mostly) was about typed data for entities. The backwards compatibility layer was removed a couple of months ago and as far as I know the concept of NG no longer exists in core.

NG is no longe relevant, but what this actually is and always was is the separation between content (NG) and config entities.

So the NG and the non-NG stuff can be removed from the issue summery?

No, but replaced with Content/Config.

@Berdir: Thanks for clearing that up.

I have updated the issue summery with the content/config part.

This issue has been given a priority for major and tagged with "beta target" and "rc deadline".
- Does that mean that related sub-issues are also priority major and tagged with "beta target" and "rc deadline"?

Once #2224833: Remove redundant ID manipulation in ConfigEntityStorage::save() goes in, then all of the public $id (or whatever key it is) can be made protected. We can revisit the entity types that were already converted, but the new conversions can make this change in their dedicated issue.

It would be nice if this issue explained that your entity type has to implement toArray() once you switch the properties to protected. Just wasted quite a bit of time on debugging while working on #2030605: Expand Editor with methods.

Per discussion with @Berdir, the most important task to move these issues forward is architectural review of the child issues.

Edit: Also, it'd be awesome if someone could incorporate the explanations on-issue of what the purpose of all this is, that'd be valuable. Suggestions on what to look for in a code review would also be valuable.

There is no separate issue for terms, so should it be as part of #2030669 - Expand Vocabulary with methods, or create a separate issue? I prefer to do on same issue, what other's have to say?

Terms were done in #2016701: Expand TermInterface to provide methods, what else would you want to do there?

Oops, I miss that.
Fields are missing in Drupal\taxonomy\Entity\Term class.

Just want to point out that most of the config entity issues linked from the original post have some work on them, and are awaiting review and maybe even RTBC. :-)

#2030595: Expand ConfigTest with methods and #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods might be 'won't fix' at this point.

Updated the "disruption" section with specifics (please double-check them).

Moved #2030645: Expand Menu with methods to the completed tasks list

Updated the IS for the new names of Field config entities (FieldStorageConfig, FieldConfig, BaseFieldOverride)

The module custom_block has been changed to block_content.

This is what alexpott said in #2030645: Expand Menu with methods comment #31:

Reclassifying as a bug since exposure of properties as public allows code to not use the API eg ->id vs ->id(). This makes Drupal more stable and prevents bugs. Nice work.

The problem is that the following Config Entities have variables that are public and they are on the are on the completed tasks list:
- NodeType
- FilterFormat
- ShortcutSet
- Migration
- RDFMapping
- ImageStyle
- Searchpage
- Action
- ConfigurableLanguage

If we want to do what alexpott is saying we need to update them. Do we open new issues and attach them to this meta issue or do we need to solve this some other way?

The issues #2030613: Expand EntityViewMode (really EntityDisplayModeBase) with methods and #2030661: Expand Tour with methods have landed.

The issue is all about encapsulation. One of the pillars of object-oriented programming. As a community we must get this fixed for drupal core. The problem is that patches made for the sub-issues are open for a long time. So I would like to make a proposition. If you write a patch for a sub-issue, then I will do a good review. Hopefully gets this issue solved before the first release candidate. At the moment there are 9 sub-issues left and probable 9 more to follow. So who will write a patch.

@YesCT: I will make new issues for them tomorrow. Thank you for the help. If you can help with some "novice" patch makers, I shall do the reviews. I would be really nice if we would finish this issue before the first release candidate.

I added this: #2396713: Figure out how to encapsulate writable settings supplied by DataDefinitionInterface.

it's blocking this: #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods, and maybe other issues in this meta.

Opened #2399965: Config entity get() and set() should invoke getters/setters, which I feel addresses a major problem with how this was implemented for config entities.

Also, __construct() modifies the properties on its own, without even calling ->set(), so that's a third code path for setting properties, but also the one that's easiest to fix (by making it call ->set()).

Added two new ones: #2525002: Make the class variables protected for Migration and #2525068: Document the class variable Node::in_preview as will stay public.

I would be nice if we could add a test that would test if there are public class variables in subclasses of ContentEntityBase and ConfigEntityBase.

Added #2527076: Make the class variables protected for Drupal\Core\Datetime\Entity\DateFormat to the TODO list.

Added #2527114: Create PHPUnit test that Entity class variables are protected or are allowed public to make sure that there will be no new public class variables added as a accident.

I do not know if we can mark this meta issue as fixed.
All class variables are now protected, but there are some issues open:

1. In #2527114: Create PHPUnit test that Entity class variables are protected or are allowed public there is a discussion if we should allow contrib module to have public class variables.
2. In #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods user mpdonadio wants a change record for the issue.
3. In #2532776: Add the undeclared variable $in_preview to the class Comment is related to this meta issue.

Related also needs to be fixed

Downgrading to normal and moving to a plan. I think everything important is tracked in its own issue now or already fixed.

Also, AFAIK, there aren't any remaining children that are actually RC deadline -- if there are though, we'd put that on the child issue.