We should make use of type-hinting for entity-parameters to make sure expected entity objects are passed in. That way we can ensure no half-baked stdclass objects or similar are passed around, maybe stemming from some legacy code.

According to the coding-standards, type hinting should only be used with interfaces. That's a good idea as it allows for changing the implementing class. Thus, for entity-generic functions we can type-hint to EntityInterface (once we've converted all entity types in core).

However, when a function or method can use entity-type specific type-hinting:

We define a new per entity-type interface and use that.

<?php
CommentInterface
extends EntityInterface { }

function
comment_publish(CommentInterface $comment) {
..
}
?>

While the CommentInterface is the same as the EntityInterface, it makes clear that a comment is expected: DX++.

Files: 
CommentFileSizeAuthor
#116 typehint-1391694-116.patch83.92 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,704 pass(es).
[ View ]
#99 typehint-1391694-99.patch83.92 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch typehint-1391694-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#99 interdiff.txt13.71 KBtim.plunkett
#96 typehint-1391694-96.patch82.28 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,475 pass(es).
[ View ]
#87 typehint-1391694-87.patch73.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch typehint-1391694-87.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#81 typehints-1391694-81.patch46.83 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,027 pass(es).
[ View ]
#8 comment_interface_proove_of_concept.patch11.62 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/comment.entity.inc.
[ View ]

Comments

aspilicious’s picture

From a dx perspective I would like to use (a) but after reading this issue I believe (c) is our best option. (b) is terrible and I alrdy was wondering why we did that.

This is kinda major with all the refactoring in place else we have to rename a bunch of code.

jherencia’s picture

I think we need to follow the coding standards, my vote for (c).

klausi’s picture

Option a) is an OOP no-no. I guess Crell would say "only over my dead cold body".

Option b) is the most extendable/generic solution: the code does not make assumptions it doesn't have to make, so a fancy module could pass special nodes to the functions/methods that look like comments. The caller is responsible that all public properties of the passed object conform to comment objects.

Option c) is a pragmatic solution that makes the code more readable. If I want to write my fancy module that passes nodes instead of comments around, I would just create

<?php
class MyCommentNode extends Node implements CommentInterface {}
?>

So I could still use the comment API.

Therefore I also vote for c).

pounard’s picture

I'm not sure we need the per-entity type interface, when do the entities class will be overrided? Anyway, let's admit it what is going to happen: if we do this, the CommentInterface must be used anywhere even in comment module itself, and never rely on actual Comment class implementation.

webchick’s picture

Are these the same coding standards that were invented like a month ago? We can change them, you know. :)

a) is the only one that makes any sense to me. I would need a much stronger reason than "because the coding standards say so" to go for c).

Tagging for the folks heading up the Learnability initiative to weigh in on.

webchick’s picture

Issue tags:+coding standards

Oh, and coding standards.

pounard’s picture

@webchick I think you're right, but where does the coding standard says we cannot use class name for type hinting?

aspilicious’s picture

Status:Active» Needs review
StatusFileSize
new11.62 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/comment.entity.inc.
[ View ]

Here is patch with all the renaming for the Comment object to CommentInterface.
It's just proove of concept to test the bot.

Well the other thingie fago mentioned is:

According to the coding-standards, type hinting should only be used with interfaces. That's a good idea as it allows for changing the implementing class.

Chances are small we are going to rename Comment to lets say FooBar but it is a valid point. But on the other hand when we change the object to FooBar we have CommentInterface $fooBar which isn't prety either...

Note: going to mark this active again when I have testbot results

Status:Needs review» Needs work

The last submitted patch, comment_interface_proove_of_concept.patch, failed testing.

pounard’s picture

CommentInterface extends EntityInterface { }

CommentInterface has no business methods, but Comment class could have some. Where I don't like this interface adding without any design purpose behind is that you mask the fact you are actually awaiting Comment classed objects, not CommentInterface objects. This would be good if we could ensure that *all* code including comment module code always relies only on CommentInterface and never on Comment class.

aspilicious’s picture

Status:Needs work» Active
fago’s picture

@pounard, #7: Read http://drupal.org/node/608152, type-hinting.

ad #3 :
I'm not sure that's valid. If we've documented to expect an entity with type == 'comment', you break the assumption. E.g. you'd loose any modifications that a module does on hook_comment_load(), but this modifications might be expected on your passed $comment somewhere later.

This is, as we do not have support for subtype polymorphism for entities. Else we'd have to invoke hook_comment_load() for an entity-type derived from 'comment', but we don't and that kind of makes CommentInterface pointless for denoting "I require an entity of type comment".

Thus, any reasons left to not go with a)?

Once we go with a), we'd hardcode the used-class, thus we could also allow creating comments via:

<?php
$comment
= new Comment();
?>
aspilicious’s picture

Ok damn I forgot you couldn't do

<?php
$comment
= new Comment();
?>

When you don't want to hardcode the class.
It's a serious WTF if we don't allow to do that because it increases the learning curve for everyone familiar with oop outside this project.

Now I lost my faith in (c)

fago’s picture

Well, with "new Comment()" we would also loose easy override-ability of the comment class, e.g. for customization purposes. So whether we want that would be a separate discussion we'd need to have.

pounard’s picture

We don't loose override-ability, we can still extend it, which is not that bad, right? But as the code exists right now, I'm definitely in favor of keeping the a) solution.

fago’s picture

If modules do $comment = new Comment;, you cannot override the used class any more, as opposed to entity_create() + entity info.

However yes, with a) you can still override the class to any class extending Comment. There is no existing code that does that type-hinting though.

pounard’s picture

Yes, s/keeping/going with/

Berdir’s picture

The problem is, since we do not use get/set methods, the CommentInterface actually tells you nothing.

The advantage of a) would be that it's self documented. You can go look up the Node class (when using an IDE, you even get auto-completion), but the interface doesn't say anything about the properties. But I agree that we shouldn't use a class for type-hints or anything else.

so I agree with c).

@webchick: Note that writing e.g. "function yourmodule_comment_presave($comment)" works *perfectly* fine. There is no technical connection between the hook documentation and an actual implementation, very much unlike what you have with interfaces and classes. The only disadvantage is that you don't get autocompletion if you're using an IDE. On the other side, this IMHO improves the self-documentation of code (hook documentation, actually). So I don't see an increased learning curve here, other than maybe having to understand what type hints are (although you can copy & paste the example code and adjust it, just like before...)

pounard’s picture

But I agree that we shouldn't use a class for type-hints or anything else.

Is there any particular reason?

klausi’s picture

The reason is tight coupling, which should be avoided when possible. If we use the class we force developers that want to extend and reuse the comment system to derive their specialized classes from Comment. PHP does not support multiple inheritance, so you cannot inherit from anything else and are heavily bound to the Comment class.

"Program to an interface, not an implementation."
"Once you depend on interfaces only, you're decoupled from the implementation. That means the implementation can vary, and that's a healthy dependency relationship."
from http://www.artima.com/lejava/articles/designprinciples.html

pounard’s picture

I know the principle and I mostly agree, but really, never use a class for type hint? Not everything is meant to be overrided. Right now for the entity system I'd tend to agree, but as long as the comment interface does not really carry anything worthwhile of being overrided (the object itself has no method, it's merely only a data carrier) there is not concrete use.

sun’s picture

I think we even have it documented somewhere, type hinting should only ever use interfaces.

We do that in several other locations throughout core already (most notably the database system, if I'm not mistaken).

Thus, c) looks most appealing and most correct to me.

pounard’s picture

There is something dumb with this convention: type hinting should always be done when it means something. This means that by default, we should type hint with classes if we can (and if no interface is there for this), then derivate to an interface if we need to make this particular piece of code swappable. But no type hiting at all just because it's a "class" is dumb per definition: type hint allows us to make certain parts of the code type safe.

Crell’s picture

webchick: No, it's coding standards we've had for about 2 years, I think. :-)

I think a few people are confused as to what type specification (I still hesitate to call it hinting given that it generates an E_FATAL) signifies. It doesn't mean "I expect an object of type X." It means "I expect an object that I can talk to using the agreed upon contract X". Specifying a class name there means "I insist that I can talk to it precisely like this class, right here, and nothing else". Specifying an interface means "I have agreed to only call methods defined by this interface, so anything you give me is fine as long as I can keep doing that."

Since even on a data object you should only be calling methods, not accessing raw properties (the current implementation still has public properties for a transitional period only; they are not supposed to remain IIRC), that logic applies to data objects just as well as behavior objects.

A few people have noted that if someone does $c = new Comment() somewhere, you cannot change it. That's true. That's one reason why our coding standards also, on the same page, discourage explicitly instantiating objects rather than using factories.

However, that's not the point. If we type specify CommentInterface, then some other developer can do this:

<?php
// This class represents comments from, say, Facebook or phpBB.
class ForeignComment implements CommentInterface { ... }

$c = ForeignComment($some_external_id);
module_invoke('comment_load', $c);
?>

If we instead specified a class of type Comment, then we couldn't do that unless we explicitly extend from Comment and then override 95% of it. The hook doesn't *actually* care that it gets a Comment. It cares that it can talk to it *as though it were a comment*.

"Looking like a comment" and "being a comment" are separate concepts, and that split is one of the chief advantages of interface-centric development.

So while I agree it looks silly to have an empty interface, I would still go with C. And who knows, there's plenty of reasons why comments could end up with comment-specific methods at some point, as could any other entity type.

pounard’s picture

I'm inclined to go with C because it makes sense here, but if there's really a coding standard saying we should not use class for type hinting, this should be removed: it's a bad advice telling people not doing type safe code.

klausi’s picture

@pounard: Yes, I have/had a hard time thinking about what entities actually are. We are used to treat entities as data junks and do a lot with public properties. We are used to treat them as data access objects, primarily as storage system and shared memory that is passed around.

But if you look at the EntityInterface that we introduced in D8 there are quite some methods, that fulfill operations on one particular entity (they are specialized on the instance), while we have more independent functionality in the controller(s). So I think that entities will emerge more and more as full business objects in D8 that also contain more logic. We have a lot of well-tested, documented and proven legacy code that we cannot just throw away and re-write. We have to integrate the new entity concept in a future-proof way, that is why we want type hinting and interfaces: we insist on proper objects that follow a contract (die, stdClass, die!).

And every time you use a class for type hinting you should ask yourself: is this really for internal use? Is there no possibility that somebody would want to use that from the outside with a different object? One of Drupal's characteristics is that it is very extensible and customizable, and in OOP only interfaces allow a maximum of flexibility. So I indeed think that if you use classes for type hinting you're doing it wrong.

@Crell: no, we will not remove (all) public properties for Drupal 8, as this would result in a complete Drupal re-write. I'm optimistic that we maybe achieve that for Drupal 11.

Crell’s picture

klausi: Keeping public properties as a temporary BC measure was what we discussed in London with pwolanin and fago. If that got dropped at some point without someone telling me, I will be most unhappy.

klausi’s picture

Hm, I had the impression that changing public property handling is out of range right now, that it is still undecided what we are going to do about it and that we fix more pressing entity issues first. I have the feeling that you could be right about removing them altogether, but I would not consider them just being "backward compatibility". They are at the very heart of how Drupal and its ecosystem works, IMO. So no, this was not dropped but I think it needs more discussion at a later point in time.

Anyway, offtopic for this issue.

pounard’s picture

And every time you use a class for type hinting you should ask yourself: is this really for internal use? Is there no possibility that somebody would want to use that from the outside with a different object? One of Drupal's characteristics is that it is very extensible and customizable, and in OOP only interfaces allow a maximum of flexibility.

Definitely, they can be overrided as the new API exists. But right now, we're discussing type hinting while the choice of making them an interface is a deeper choice, and should not be considered lightly. Making Comment responding to an interface does not only impact the choice of doing type hinting, but also forces us to re-read each and every single one of the core code lines that uses those entities to ensure they really use only what the interface gives us as contract. It's long hard work. Placing an interface there and not using it as is it would be a huge Drupal WTF. That's why I would not disagree with putting Comment class as type hint until this hard work has been done: it makes the API much more consistent until we are sure it is stable.

So I indeed think that if you use classes for type hinting you're doing it wrong.

Type hinting is a generic PHP facility to make type safe code; it's never wrong to type hint with a class: it's making your code type safe. Don't get me wrong, PHP is not type safe per essence, but type hinting brings it a bit more towards it. I'm not discussing this as the entity matter, but in a more general matter: Forbidding type hinting using classes is a definite mistake.

Crell’s picture

pounard: We should be forward thinking. Right now public properties are there, and we cannot stop code from fondling them. However, moving toward using the interface and methods is what we want to do, even if it's not yet mandated by code. Declaring an interface hint rather than class hint does no harm to that code now, but makes it clearer that we're trying to leverage an interface; interfaces cannot declare properties, only methods. If anything, it signals to developers "hey, you really should be using the methods on the interface now, OK? Fair warning, dude!"

pounard’s picture

I totally understand that, but this issue definitely raised some red flags about coding convention. Still is really odd to me to type hint with an incomplete interface.

fago’s picture

klausi: Keeping public properties as a temporary BC measure was what we discussed in London with pwolanin and fago. If that got dropped at some point without someone telling me, I will be most unhappy.

No, we were talking about introducing getters/setters in addition to the current public properties. No one agreed upon making properties protected nor really discussed it - I remember that you stated it as your goal though. Anyway, it's a separate discussion for which I started #1301106: Rely on methods to access entity properties [policy, no patch] some time ago, so let's please have the discussion over there.

Anyway, please - do not forget we are working on Drupal which is a very modular system. We won't ever have interfaces that define all getters and setters for all properties, as properties in Drupal are module-defined and so dynamic. This is why code has to check for $entity->entityType() == 'comment' to make sure the required properties are there - instead of being able to just rely upon a CommentInterface.

That said, I think the code example of #24 is rather dangerous: It goes and passes $entity around to functions that assume that the entity is of type 'comment', that it's id is the 'cid' and that it's stored using the database-controller in the 'comment' table (as entity-info tells so) - but it isn't, it's something else. Thus once the called code relies upon the entity being of type 'comment' things are going to break.

However, type-hinting to CommentInterface suggests to developers that code-snippets like #24 are something the system was designed for - but it was not. Thus, we should not go with option c) in order to avoid people having wrong assumptions and writing broken code.

That said, I've to agree with pounard that there is no point in type-hinting to an interface if we do not support passing multiple entity types in. Still, once we've real interfaces in future and that's solely what the function relies upon, I'm happy to go with c). But as long as that is not the case we should not pretend we can handle something we can't, thus stay with a).

Crell’s picture

fago: Should we not be pushing toward what we want the system to be, rather than what the big pile of legacy code assumes the system was?

(Eg, I'd consider any code that still looks at $comment->cid rather than $comment->id() to be buggy and in need of a patch.)

webchick’s picture

Until the system actually is what we want it to be, I don't see the DX trade-off of not documenting what it actually is as beneficial. What cluestick am I missing?

Crell’s picture

webchick: Because it "is" many things. It's an object, it's an EntityInterface, it's a Comment, and it's potentially a CommentInterface. All of those are what that variable "is". It's what we want developers to think of it as.

And yes, we do now have a Drupal.org issue that comes to down to "that depends what the definition of 'is' is." :-)

webchick’s picture

Oh, shoot me now. :)

klausi’s picture

Also, if we use classes for type hinting we explicitly forbid that one class could replace more that one entity type class.

<?php
function hook_entity_info_alter(&$info) {
 
// 'class' property does not exist yet, but let's assume this will be possible
  // at some point.
 
$info['node']['class'] = 'MyNodeAndComment';
 
$info['comment']['class'] = 'MyNodeAndComment';
}
?>

I want to replace node and comment classes with my custom class. But what class do I extend now? If I do

<?php
class MyNodeAndComment extends Node {}
?>

I will get fatal errors because the comment API expects objects of type Comment.

fago’s picture

fago: Should we not be pushing toward what we want the system to be, rather than what the big pile of legacy code assumes the system was?

Generally, yes. But first we need to know how the system should look like.

As I pointed out in #32 CommentInterface we'll never be able to warrant that stuff like $comment->get('module_property') is going to work. So what's the point in having it?

The main advantage I see, is that you are not urged to extend from Comment if you want to override the Comment class of comment entities.

(Yes there are many other good reasons to use interfaces in general, but I'm talking about the specific and very basic case of a function requiring an entity of type 'comment' only.)

webchick: Because it "is" many things. It's an object, it's an EntityInterface, it's a Comment, and it's potentially a CommentInterface. All of those are what that variable "is". It's what we want developers to think of it as.

Yep. So what should the developer think? - It should be clear to the dev that the function requires entities of type comment.
We could achieve that with both a) and c) though, whereas for c) we would have to clearly state at the interface that is supposed to be only implemented by entities of type comment. Once we consistently do so for all entity types and properly document it at every interface, it would work out I guess.

E.g.

<?php
/**
 * Defines an interface for entities of type 'comment'.
 */
CommentInterface extends EntityInterface { }
?>

That way it should be clear it's not supposed to have something like #37, a node being passed in as comment.

(Eg, I'd consider any code that still looks at $comment->cid rather than $comment->id() to be buggy and in need of a patch.)

I'm not sure about that, but let's leave that discussion to #1301106: Rely on methods to access entity properties [policy, no patch].

That said, a) would allow for auto-completion of public properties in IDEs, what c) does not.

fago’s picture

ad #37: Generating fatal-errors if someone tries to pass in a node for a comment is exactly what the type-"hint" should achieve. It's the whole point of the type-hint to make it safe to rely upon the entity being of type 'comment'.

klausi’s picture

Sure, but in my example the class is supposed to be of type comment, I just have no possibility to state it as such. That can only be achieved by interfaces.

Crell’s picture

Re #38:

That way it should be clear it's not supposed to have something like #37, a node being passed in as comment.

That's the key philosophical difference. Hinting on Comment says "it must be this class, and therefore must not be any other existing class." Hinting on CommentInterface says "it must be something I can talk to using these methods. Beyond that, I don't care."

If someone has a use case like kalusi lists, or I have in #24, why shouldn't it be possible if the code would still work? Why should we make it a fatal error to even try?

The class name specification is inherently and by definition more restrictive and more limiting than the interface definition. And I have a hard time seeing why we should be more restrictive than necessary.

Also, as I noted in #1301106-4: Rely on methods to access entity properties [policy, no patch], there's no reason we cannot add convenience methods to specific entities like $comment->getParentEntity(). A CommentInterface would be the perfect place to put that, because then a function/method can say, very simply, "whatever it is you're giving me, I expect to be able to call getParentEntity() and not die", because that is all it cares about.

tstoeckler’s picture

Also, as I noted in #1301106: Rely on methods to access entity properties, there's no reason we cannot add convenience methods to specific entities like $comment->getParentEntity(). A CommentInterface would be the perfect place to put that, because then a function/method can say, very simply, "whatever it is you're giving me, I expect to be able to call getParentEntity() and not die"

That is I think the important point here.
Doing

<?php
interface CommentInterface extends EntityInterface {}
?>

(i.e. an "empty" interface) is, at least in theory, superfluous, as you cannot rely on a SomethingThatImplementsCommentInterface having anything more than SomethingThatImplementsEntityInterface. So you might as well just use the latter. Because interface-wise you could just as well pass in a node to comment_save() and everything would work. In theory!

In practice, of course we DO do things like $comment->pid, which obviously doesn't work if $comment is in fact a node. But that is a problem with our current code. (At least if we want to support pluggable comment classes, which I think most people would agree we do.) As Crell correctly points out, in a perfect world, we would be doing $comment->getParentEntity(). But for now that's just a pipe dream, and I don't think we want to hold this issue up on all entity-related ugliness being rid from Drupal core.

Again, I don't think many people would argue that, in a perfect world, if you had a CommentInterface which actually defined what you need to have to be a comment, that we should not type-hint CommentInterface.
But as it stands, we are relying on the very specific Comment class, so we might as well tell people the truth.
Maybe we can open a (perhaps major) "Define a proper CommentInterface" issue that could deal with exactly that. (For now we only have comments, so it wouldn't be flooding the major queue.)

bojanz’s picture

<?php
function comment_save(Comment $c) {
 
// Did I die?
}

class
Comment {}
class
AwesomeComment extends Comment {}
$c = new AwesomeComment;
comment_save($c);
?>

That works. So if we disregard the OOP-purity argument, a) is still an option. I can extend the comment class and add my stuff.

b) feels a bit too generic for me.
c) does have the purity point. However, the fact remains that it adds a requirement for each entity class to have a matching entity interface (most likely empty), in a separate file (because of PSR0), and that feels like a DX regression to me (more boilerplate).

Also, while a) makes the override-entity-class situation possible, I think that in most cases the controller is the one class that will actually be overridden.
I remember Crell's words in #1184944: Make entities classed objects, introduce CRUD support:

- The job of an entity is to "be" something.
- The job of a controller is to "do" something.

which sound relevant.

Crell’s picture

How is that a pipedream? Comment is already a class, isn't it? Adding that method would be a 20 minute patch if we were so inclined.

I really do not understand why people in this thread are so inclined to block functionality from even being possible at a language level...

pounard’s picture

@Crell we are not blocking functionality: it's not sane to type hint with an interface while the rest of the code may not fully prepared to it, that is all. That is also not sane not to type hint at all. It's not a functionality if the rest of the code isn't fully prepared to it, it's a bug.

fago’s picture

That's the key philosophical difference. Hinting on Comment says "it must be this class, and therefore must not be any other existing class." Hinting on CommentInterface says "it must be something I can talk to using these methods. Beyond that, I don't care."

But if the message of the type hint should really be "I require an entity of type 'comment'" the type hint CommentInterface what tell me something different?

Anyway, let's consider a potential use-case of c), with which me and klausi came up with when we discussed this issue in person:

* I've a library that e.g. talks to X's api and gives me X's comments. It's OOP so the actual comment retrieved from the library is classed.
* I exchange the storage controller to retrieve and update the comments at X instead.
* So now, by having the CommentInterface I might be able to replace comments with the comments retrieved from the library X, but those comments are based on a class of library X. Thus, I could sub-class that class and make it implementing the CommentInterface. Now, I need to be able to tell library X to use my custom class for returning Comments. Given that, I can return those comments while having comment module working with them.

I initially thought that gives a valid use-case for c). However, I realized that library X probably won't allow me to use my custom class for retrieving comments. Thus instead, I think it's much more likely that the solution would take the comment from X and map its data to a new Drupal $comment instance anyway. Thus, you won't need to replace the Comment class.

That said, I yet fail to see what the advantages of going with c) would be. If anyone could come up with potential use-case for c) I'd be happy to hear.

sun’s picture

Issue tags:+Entity system

I've changed my mind. Partially. For pragmatic reasons.

Work on #1361232: Make the taxonomy entities classed objects and related "entityfication" issues changes a lot of code for entity classes.

Based on the current (in)decision, we're completely leaving out function signatures in those patches.

This means that it's going to be a pain to go through each of those modules once again to find the locations where to add the (minimally) necessary type-hinting. :-/ (minimal, because those patches do not touch all module API functions that exist)

So. While I'm still in favor of c) TaxonomyTermInterface, it's going to be way more pragmatic and way more simple to find and locate the functions through-out the entire core code base where type-hinting might need to be changed, if we start right away by doing and adding a) TaxonomyTerm now.

Essentially:

  1. Type-hint with the entity class name for now; e.g.,
    function taxonomy_term_save(TaxonomyTerm $term) {
  2. Mark this issue postponed, tag it as "revisit before release", and get back to this discussion when we've gained more data, knowledge, and experience throughout Drupal core; i.e., after making more progress on the new Entity system, entity methods, Bundle system, Entity property API, Field API, etc.pp.
  3. Depending on discussion results, possibly relax the parameters via TaxonomyTermInterface or EntityInterface. Due to 1), this will be a quick and simple search & replace at that point.
bojanz’s picture

Sounds like a plan to me.

aspilicious’s picture

EDIT: removed my comment

Crell’s picture

I wasn't really supportive of sun's #47, but he did point out that we have #1393358: [META-12] Review 'revisit before release candidate' tag, which means we may actually get back to it after all. :-P Given that we don't want to block the entity conversion patches, I can live with sun's proposal.

pounard’s picture

Even if I'm pro a) any of the a) or c) solution would be fine, so sun's proposal sounds good to me too.

webchick’s picture

Status:Active» Postponed
Issue tags:+revisit before beta

Let's do it.

Dave Reid’s picture

This is sort of blocking #1920688: Support multiple instances of the same token type in token replacement which depends on a reliable way to determine if a random object is an entity, and of a specific entity type. I went with using instanceof Node and instanceof Term in the token replacement.

plach’s picture

I'm afraid that #1810370: Entity Translation API improvements will force us to use EntityInterface on every entity. See also the issue summary in #1818556: Convert nodes to the new Entity Field API.

tim.plunkett’s picture

Shifting the DX burden on to entity type providers, and off of everyone else, would be great. Requiring an interface for an entity type would be great. And 90% of the time, they'd be empty.
Views already has one, and it allows the Views UI to even function.

EDIT: What I mean is, I vote for C) from the OP

plach’s picture

C) would force every entity type to define an EntityLanguageDecorator instead of just working. I don't think it's a worth trade-off.

tim.plunkett’s picture

@plach, is that currently true? Or just will be in the future? Is there an issue? Because if that was in core, Views UI would be broken right now, as it relies on a entity-type-specific interface.

Because, yes, it does break the use of entity-type-generic decorators, but that's a hack.

Without entity-type-specific interfaces, we can't have entity-type-specific decorators.

plach’s picture

@plach, is that currently true? Or just will be in the future? Is there an issue?

There's #1810370: Entity Translation API improvements. In IRC we agreed to try and rely on a global decorator, I explicitly asked you if you were ok with that approach :)

Without entity-type-specific interfaces, we can't have entity-type-specific decorators.

If this is absolutely needed, then we are making supporting multilingual for any entity type out-of-the-box impossible. We'll have to choose, I guess.

fago’s picture

Because, yes, it does break the use of entity-type-generic decorators, but that's a hack.

Well, we could support defining entity-type specific language decorators? I'm not convinced we really need this for every entity type, but maybe just for entity-types that want to define their own interfaces?

plach’s picture

Well, we could support defining entity-type specific language decorators? I'm not convinced we really need this for every entity type, but maybe just for entity-types that want to define their own interfaces?

I agree that probably not every entity out there will need a language decorator, but it'd still be nice to able to support any entity-type out of the box, especially because in 99,9% of cases an entity-type-specific language decorator would be nothing but:

<?php
class AwesomeEntityLanguageDecorator extends EntityLanguageDecorator implements AwesomeEntityInterface {}
?>

Because, yes, it does break the use of entity-type-generic decorators, but that's a hack.

I'm not sure I can get behind this statement: IMHO the ELD is an example of legit use case for entity-type-generic decorators. There might be others we are not thinking about right now.

plach’s picture

Answering here to @webchick from #1818556-196: Convert nodes to the new Entity Field API since it's relevant:

[Replacing Node with EntityInterface in node type hints] is really unfortunate. :( I understand that it gets us more flexibility, but at the cost of DX. :(

A foreword: #1818556: Convert nodes to the new Entity Field API was committed despite @webchick's suggestion of introducing a NodeInterface to make type hints more meaningful. We can still roll it back, introduce the NodeInterface per C, and commit again the updated patch. But we need to do it now because otherwise it will be a real pain to do that, given how widespread the EntityInterface usage is. AAMOF in that case it won't be possible to search/replace stuff. As a consequence I believe now we have to finally decide between B and C, as A is no longer on the table since it makes using decorators really hard.

I think going for C makes sense only if we start providing actual entity-type-specific methods, as suggested by @Crell in #41. Otherwise I think it's easy to turn all the arguments above in favor of C to be in favor of B, since the contract entities as data objects actually respect is the same for all entity types we have in core right now. The difference is in the actual data they hold, which cannot be fully enforced through an interface due to horizontal extensibility. In this case we would just be reducing flexibility with the sole advantage of having stronger typing. I dont think we'd have a regression in terms of DX, since IMHO the following examples say exactly the same thing to a D7 developer: we need a node here. Also in D7 if you pass a comment instead of a node you'll likely notice pretty soon ;)

<?php
function node_view($node) { /* ... */ }
function
node_view(EntityInterface $node) { /* ... */ }
function
node_view(NodeInterface $node) { /* ... */ }
?>

OTOH if we do start adding proper methods on entity-type-specific interfaces (not business-logic stuff, since that's the job of controllers), then we have a new contract and the argument above ceases to be valid. If we decide to go this way, I'd be ok with allowing generic decorators when the entity type has not a specific interface, and enforcing entity-type-specific decorators when it does. To sum up allowing B and C to live together depending on the business logic implemented by the entity type.

Please let's a have a final call on this so we can move on with the NG stuff.

Berdir’s picture

Status:Postponed» Active

As commented there as well, a NodeInterface would not have worked without also introducing a NodeBCDecorator subclass of EntityBCDecorator that would do nothing else than exend it and implement the (empty) NodeInterface.

We access our data using fields/properties, and not methods. There's nothing that we could define in NodeInterface that's not already in EntityInterface, the only purpose it would have would be to say this needs to be a node, not a comment (which you can also see in $node, as you could in 7.x, just like plach said). Unlike using Node in the type hint, it doesn't give you autocomplete on what properties that entity has.

I don't think it would be that hard to go back to "Node $node" with search and replace. "EntityInterface $node" and similar ones for @param and @return should find and match most of them.

IMHO, the question is, can we use Node $node while still having a sane Entity Translation API and if not, is the advantage that NodeInterface gives us (type hints and also is this entity an X checks in Code) worth the overhead of having to subclass and provide entity type specific wrappers/decorators. Edit: This is something that can be answered per-type, so contrib entity types could stick to EntityInterface while more frequently used core types could provide an Interface.

plach’s picture

I don't think it would be that hard to go back to "Node $node" with search and replace. "EntityInterface $node" and similar ones for @param and @return should find and match most of them.

Yep, probably, although I vaguely remember that even doing the search/replace from Node to EntityInterface was not as straightforward as I originally thought.

IMHO, the question is, can we use Node $node while still having a sane Entity Translation API

Well, I think the question should be reformulated: can we use Node $node while still having sane support for decorators in general? We tried this approach in the node ng issue with the BC decorator, and my answer is no: we had to extend the Node class and copy huge chunks of code from the generic BC decorator class. This is not what I'd define sane.

plach’s picture

Unlike using Node in the type hint, it doesn't give you autocomplete on what properties that entity has.

Which I'd almost tend to see as an advantage: currently we provide broken discoverability, since all dynamically-added fields are not listed by IDEs, at the cost of a horrible hack, that is manually unsetting all the defined properties when instantiating an entity object. This is also a very fragile approach because we are not guaranteed that every entity type out there will bother with doing it, and in that case it would screw magic accessors up.

We can achieve the same with explicit accessors in a more reliable and standard way, if we really wish to.

Crell’s picture

The way I see it, the reason to make different entity types different entity types is that they do have different business significance that is intrinsic to the entity type. If there were not the case, they wouldn't need a different entity type. (Different *configuration* is what bundles are for.)

It's possible that sometime in the future we're able to make the entity system so flexible (without making it equally incomprehensible) that all functionality becomes configurable (by some mechanism other than hacking field data), in which case entity types and bundles/subtypes merge into the same thing. That's not the case in Drupal 8, though, so that's not relevant for the time being.

I think if we setup per-entity-type interfaces and then looked into it, we probably would find reasonable additional methods to add on each type. For instance, $user->checkPassword() (since password checking now has to happen post-load, not pre-load), $comment->getParentComment(), $comment->getAttachedEntity(), $product->getSkus(), etc. While some of those would absolutely be possible to do through normal generic entity methods, having those there improves DX.

It's true that doing it that way would privilege the original implementer of an entity type in his ability to define such utilities over other people extending it. I don't see that as a problem. In fact, I see it as a good thing, because the person defining the existence of the entity type should be responsible for justifying its existence and thinking through the reasonable, predicted, expected, intended use cases. That doesn't mean someone else can't write visitor classes that do other fun things with an entity; they've always been able to and that should remain. But those sort of helper methods that are not generic are good for DX, and a good architectural tool to think-through a design.

Having the interface there invites us to ask the question "so what should be on it?", which I believe is a good question to be asking.

tim.plunkett’s picture

I'm staunchly pro-interface here, and think that if decorators are preventing us from using entity-type-specific interfaces, then the use of decorators are wrong.
Also, +1000 to #65, thanks Crell.

Does anyone else have a reason to not have interfaces, other than "our decorators won't work"?

plach’s picture

Does anyone else have a reason to not have interfaces, other than "our decorators won't work"?

Decorators won't work with A, instead they will easily work with C although in the case of the Entity Translation API we'll lose automatic support, which I can live with if we do #65. What I proposed in #61 is allowing both B and C, depending on whether an entity type defines a specific interface.

Do we want to allow entity types to only implement the EntityInterface or are we officially discouraging this approach?

Edit: to be even more clear: does anyone have a valid reason to go with C if we provide empty entity-type-specific interfaces, other than stronger typing?

fago’s picture

I agree with #65, while entity-type specific interfaces might not be required in any case a) there are cases where they will be needed and b) as #65 clarifies it's a good thing to have anyway.

The downside of it is that every translatable entity type would have to provide two classes:

Node extends Entity implements NodeInterface
NodeLanguageDecorator extends EntityLanguageDecorator implements NodeInterface

plach’s picture

NodeLanguageDecorator extends EntityLanguageDecorator implements NodeInterface

Yes, in this case we could use __call in EntityLanguageDecorator so that NodeLanguageDecorator can be just an empty class.

plach’s picture

Given the last comments, can we agree on a "Stick to the implemented interface" policy for type hints? We go with C unless the entity type implements just the EntityInterface?

Berdir’s picture

__call() doesn't really help much I think. It's not necessary when NodeInterface is empty and if it's not then NodeLanguageDecorator needs to define the methods explicitly anyway. We would also need to override the method that returns the decorator on Node and return NodeLanguageDecorator instead.

plach’s picture

@Berdir:

Yeah, right, scratch that :)

Crell’s picture

I don't understand how NodeInterface precludes translation... And if it does, I don't understand why the design flaw is with interfaces rather than with translation.

plach’s picture

@Crell:

NodeInterface prevents from providing a generic EntityLanguageDecorator, i.e. a wrapper that stores the language to default to when accessing entity fields in the active workflow. You can read more about it in the issue summary of #1810370: Entity Translation API improvements. If we go for B we can have a generic decorator that works for every entity type out of the box, if go for C we need every entity type to provide its own decorator extending ELD and implementing its specific interface.

Anyway, translation is really not the problem here. If we do #65 providing a specific decorator is simply the right thing to do and I won't argue we have to go for B in this case. What I'm arguing is that going for C with empty/marker interfaces would be a big loss in flexibility not justified by the type checks this would make possible.

msonnabaum’s picture

Commented on #1810370: Entity Translation API improvements, but I'd just like to add here that not being able to have entity type interfaces prevents us from developing any kind of domain model, and is not a reasonable limitation.

I'm obviously in favor of c), but I'd rather see a) before we seriously considered b).

webchick’s picture

I concur. I'd like to understand more what about translation makes it seem like a viable option to strip any and all DX sanity away from our APIs.

plach’s picture

First of all I'd like to thank @msonnabaum for taking the time to go and read the Entity Translation API issue. Answering #1810370-22: Entity Translation API improvements here, since it's relevant:

I'd like to see some of our entity classes turn into actual domain models, and this issue would prevent that. Forcing code to type hint EntityInterface almost defeats the purpose of type hinting it in the first place. I would consider this limitation a total non-starter.

I understand this argument, but B does not prevent domain models from actually being implemented, while specific type-hinting prevents generic decorators of any kind from ever being used. I realize B is the less attractive solution in terms of DX, and I hate to be in the position of having to push for it, but exchanging flexibility with theoretical purity IMHO would be very short-sighted. In real life how often do we expect to pass a comment to node_view()? In D7 this has never been an issue.

I'm obviously in favor of c), but I'd rather see a) before we seriously considered b).

If there is a consensus around this position, I strongly recommend every one to go and read #1810370-11: Entity Translation API improvements and suggest a solution not involving decorators of any kind, not even the entity-type-specifc ones implied by C. Otherwise the DX gain we'll have here will be heavily counterbalanced by the DX loss we'll have there. And this won't concern only multilingual sites unfortunately.

@webchick:

I concur. I'd like to understand more what about translation makes it seem like a viable option to strip any and all DX sanity away from our APIs.

There is a well-crafted issue summary in #1810370: Entity Translation API improvements that explains why we think a generic decorator might be a good solution to improve DX when dealing with multilingual entities. I tried to summarize the summary in #74. I don't think it makes sense to replicate it entirely here.

If, as it seems, there is consensus that type-hints should be entity-type-specific, I think C is the only viable option, although I must say I'd be very disappointed if we ended-up having just marker interfaces, instead of meaningful ones.

Going for A would appear to me as deliberately ignoring the needs of an official initiative and good software design principles, with no actual advantage over C. In #61 I pointed out that none of the solutions discussed here can be considered a DX regression wrt D7.

plach’s picture

@msonnabaum:

Would the following policy satisfy you?

"Stick to the implemented interface" for type hints: we go with C unless the entity type implements just the EntityInterface.

Edit: A clarification:

[...] not being able to have entity type interfaces prevents us from developing any kind of domain model, and is not a reasonable limitation.

I never said we should not have entity-type-specific interfaces, I'm just proposing to not enforce them through type hints, unless they are not empty.

tim.plunkett’s picture

Right now we're adding methods to entity classes that aren't backed by an interface. That is a problem.

Here are entity types with their own methods that need to be fixed:

  • Block
  • Breakpoint
  • BreakpointGroup
  • CustomBlock
  • DisplayBase (abstract, already backed by an interface)
  • EntityDisplay
  • FilterFormat
  • MenuLink
  • PictureMapping
  • Tour
  • View (already backed by an interface)

If you remove the interface Views is using, then Views UI will cease to function.

Furthermore, it means that entity types are not actually swappable. If there is no interface, you would have to directly extend the class you want to replace, which means very tight coupling.

plach’s picture

Well, then all of the above entity types deserve a dedicated interface and we need to type-hint them accordingly. But as @fago was saying, not every entity type out there might need a specific interface and in that case I'd just type-hint it with EntityInterface, which would avoid the need to define an empty specific decorator and allow to use the generic one. I think this could be a reasonable compromise.
If this is still felt too much, I'll happily accept strict C and empty interfaces for every entity type rather than going for A.

Furthermore, it means that entity types are not actually swappable. If there is no interface, you would have to directly extend the class you want to replace, which means very tight coupling.

+1000

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new46.83 KB
PASSED: [[SimpleTest]]: [MySQL] 54,027 pass(es).
[ View ]

So, here's a patch to add interfaces. Didn't switch the typehints.

Berdir’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/FeedInterface.phpundefined
@@ -0,0 +1,17 @@
+ * Contains \Drupal\aggregator\Plugin\Core\Entity\FeedInterface.

Hm, the interfaces aren't plugins, so why put them in the plugin directory?

+++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Plugin/Core/Entity/BreakpointGroup.phpundefined
@@ -173,12 +162,7 @@ public function addBreakpointFromMediaQuery($name, $media_query) {
-   * Adds one or more breakpoints to this group.
-   *
-   * The breakpoint name is either the machine_name or the id of a breakpoint.
-   *
-   * @param array $breakpoints
-   *   Array containing breakpoints keyed by their id.
+   * {@inheritdoc}

Is this change part of this patch for a reason or did it slip in accidently? Looks unrelated...

tim.plunkett’s picture

I put the interfaces in the same directory for two reasons.
No need to use them in their entity class
Because we'd have a conflict with the block plugin BlockInterface that I didn't feel like dealing with :)

Those docs should be moving from the entity to the interface they are part of.

webchick’s picture

-class Block extends ConfigEntityBase {
+class Block extends ConfigEntityBase implements BlockInterface {

Awesome! That makes a TON more sense to me. I understand that in some cases these will be stupid interfaces that won't add anything over ContentEntityInterface, but I also think this gives us a lot of architectural flexibility to introduce special-casing where it's warranted, in a standard OO way.

webchick’s picture

Hm. However, I agree with Berdir. Putting things that aren't plugins into /core/modules/X/lib/Drupal/X/Plugin/ is a recipe for confusion.

In a quick scan, the existing BlockInterface seems like it's the exact justification for this move in the first place? So is there really a conflict to resolve here?

Berdir’s picture

I wasn't aware that we already agreed on using @inheritdoc, is that the case? That's the main reason I was confused by those changes.

@webchick: (BlockInterface) That's where it gets interesting ;) The existing BlockInterface has nothing to do with entities, it's the block plugin interface that defines the block plugin. I'm not sure what the exact state there is, but it could be argued that it's the block entity/interface that is misnamed, as it is, as I understood, configuration of a block plugin instance. But that is right now also more or less understood as a "block" (in a UI sense, an thing that is placed in a region).

Also, as a reminder, the reason the initial decision was against interfaces was that both the old and the new Entity Field API relies on properties (in case of the new Entity Field API, properties backed by lot of magical methods). That does not work with interfaces. Which means that by moving to NodeInterface instead of Node as the type hint, we loose IDE auto-complete and api.drupal.org support for $node->uid, $node->status and so on. That is true for both using EntityInterface and NodeInterface. So that leaves us with three options:

a) Live with it. Most of the time, we're actually interested in standardized methods like id(), label(), bundle(), getRevisionId(), language() and so on anyway.
b) Rely more on get('status') which isn't really better because you still don't know what kind of properties exist but it at least tells you what get() returns. Because with the new Entity Field API, that is a Field object and not a raw value.
c) Add things like getUser(), getStatus() and so on to NodeInterface which has two disadvantages: 1) A lot of almost identical getSomething() methods that need to be duplicated for translation decorators. 2) It means that, again, hardcoded fields (like uid, status, ..) are accessed in a different way than configurable fields (like body, tags, field_my_value).

The advantage is that we could, if we want to, get rid of that ugly and confusing init()/unset() code in Entity NG classes, which we mainly have to support IDE auto-complete.

tim.plunkett’s picture

StatusFileSize
new73.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch typehint-1391694-87.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Well I renamed BlockInterface to BlockPluginInterface.
Also had to deal with EditorInterface (both of them?!).

Moved out of Plugin\Core\Entity

I favor DX of methods over DX of properties that should be protected anyway.

EDIT: Because we're moving BlockInterface around and then replacing it with a new class, the diff looks very odd. Apply it and all will be well :)

xjm’s picture

Okay, wow, this wasn't in my radar at all.

msonnabaum’s picture

@Berdir - we should do C. If a property is guaranteed to be there, it should have an accessor method. If it's optional, then it makes sense not to have an accessor method. That's not inconsistent, that tells me something important about the object. Sure, its more work for translation decorators, but it's explicit and provides a much clearer DX.

plach’s picture

As soon as we have an explicit NodeInterface I'm definitely +1 for #89.

tstoeckler’s picture

Putting things that aren't plugins into /core/modules/X/lib/Drupal/X/Plugin/ is a recipe for confusion.

Just a note that it's currently a common pattern in core to put plugin base classes, which are not themselves plugins, into the plugin directory next to the actual plugins. Forthe plugin interfaces this is sometimes done and sometimes not, unless I'm mistaken.

Don't know whether I find that good or bad, but due to this putting the NodeInterface etc. into the plugin directory next to Node would be more consistent than putting it elsewhere currently.

Crell’s picture

I've no problem with intelligent addition of extra utility methods to various entity classes where they make sense, as long as it's "where it makes sense" and not "add methods for all the things!" (For nodes, getting the author totally makes sense.)

plach’s picture

Given the last comments, can we agree on a policy of always defining an interface at least providing accessors for non dynamically-provided fields when defining an entity type (and type-hint it accordingly)?

Node would be:

<?php
interface NodeInterface extends EntityInterface {

  public

setAuthor(UserInterface $user);
  public
getAuthor();

  public

setStatus($status);
  public
getStatus();

  public

setCreated($created);
  public
getCreated();

  public

setChanged($changed);
  public
getChanged();

 

/*
   * Should we move these into EntityInterface?
   */

 

public setRevisionUser(UserInterface $user);
  public
getRevisionUser();

  public

setRevisionTimestamp($timestamp);
  public
getRevisionTimestamp();

 

/*
   * The following should go away as part of the upcoming Entity API
   * refactorings.
   */

 

public setSticky($sticky);
  public
isSticky();

  public

setPromote($promote);
  public
isPromote();

  public

setComment($comment);
  public
getComment();

}

?>

Accessors for ids/label/language are/should be in EntityInterface.

tim.plunkett’s picture

The last submitted patch, typehint-1391694-87.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new82.28 KB
PASSED: [[SimpleTest]]: [MySQL] 54,475 pass(es).
[ View ]

I think adding accessors is great, it will help with #1959610: Remove public properties from entity classes.
But we need the interfaces first.

Rerolled for Field/FieldInstance

fago’s picture

Awesome! That makes a TON more sense to me. I understand that in some cases these will be stupid interfaces that won't add anything over ContentEntityInterface, but I also think this gives us a lot of architectural flexibility to introduce special-casing where it's warranted, in a standard OO way.

Yes!

I've no problem with intelligent addition of extra utility methods to various entity classes where they make sense, as long as it's "where it makes sense" and not "add methods for all the things!" (For nodes, getting the author totally makes sense.)

I'd second that. Then, of course those utility methods should go into the entity-type specifc interface also.

Don't know whether I find that good or bad, but due to this putting the NodeInterface etc. into the plugin directory next to Node would be more consistent than putting it elsewhere currently.

To me having the NodeInterface besides the Node class would make most sense also. It's obviously connected to the Node plugin, so I think it's ok to have in the plugin directory.

Berdir’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -0,0 +1,17 @@
+ * @todo.
+ */
+interface FeedInterface extends ContentEntityInterface {

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.phpundefined
@@ -0,0 +1,17 @@
+ * @todo.
+ */
+interface ItemInterface extends ContentEntityInterface {

I think we should replace these @todo's with a single line sentence for each interface, doubt it will get in if that's the first thing you see.

Alternatively, we declare this as a policy issue and open follow-up issues for each entity type where we add it. We'll need one per entity type anyway, to convert existing type hints, discuss methods on the interface and so on. The only difference would be that we also only add the interface there.

tim.plunkett’s picture

StatusFileSize
new13.71 KB
new83.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch typehint-1391694-99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Okay, removed all of the leftover @todos.

I've already moved all public methods to the interfaces, so no need for a separate issue for each for this patch itself.
We will need some cleanup to remove all of the EntityInterface type hints that have spread around.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Ok, let's do this. I think we have an agreement to live with whatever disadvantages those interfaces will bring as the advantages clearly overweight. The main one (requiring per-entity type decorators) might go away according to the current discussion in the entity translation API meta issue, so yay!

The patch is good, just adds the interfaces and moves around some documentation.

As discussed, we still need the separate follow-ups I think to discuss which methods each interface should get and to actually those interfaces. Which in some cases will not be a trivial search and replace, as we e.g. need to add NodeBCDecorator, but at least that on is temporary.

Status:Reviewed & tested by the community» Needs work
Issue tags:-DX (Developer Experience), -Entity system, -coding standards, -revisit before beta, -Increases learning curve

The last submitted patch, typehint-1391694-99.patch, failed testing.

tim.plunkett’s picture

#99: typehint-1391694-99.patch queued for re-testing.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Random failure.

fago’s picture

Yeah, I like where this goes as well. Differentiating between interfaces for the plugins and for the config entities is also very important, so that's a good step into the right direction.

tim.plunkett’s picture

#99: typehint-1391694-99.patch queued for re-testing.

xjm’s picture

Status:Reviewed & tested by the community» Needs review
Issue tags:+Needs issue summary update

Love the DX improvement of this patch. Since we're adding a bunch more autoloaded files, though, let's get benchmarks to measure the impact, if any. We need to get in the habit of doing this during feature freeze. Presumably we'd want to test a page with a bunch of entity types, like a view of articles or such.

Also, it would be good to update the summary to clarify which resolution we're going with here, and what the DX improvements are.

Finally, let's get those followup issues linked here. :)

msonnabaum’s picture

I feel like we should not worry about the impact of more autoloaded files if it makes sense architecturally.

xjm’s picture

Tag monster.

Thanks @msonnabaum!

xjm’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs issue summary update

And @tim.plunkett!

xjm’s picture

#99: typehint-1391694-99.patch queued for re-testing.

tim.plunkett’s picture

xjm’s picture

Issue tags:+Needs followup

Oh right, the followups per #100.

webchick’s picture

This looks great, and I'd love to commit it, but there are currently at least two RTBC majors that look like they're almost certainly going to conflict with this:

#1818560: Convert taxonomy entities to the new Entity Field API
#1869562: Avoid instantiating EntityNG field value objects by default

Don't have time right atm to dig in and see if that's the case, but don't want to force re-rolls of majors for normal-level refactoring (knowingly, at least).

Berdir’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+DX (Developer Experience), +Entity system, +coding standards, +revisit before beta, +Increases learning curve, +Needs followup

The last submitted patch, typehint-1391694-99.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new83.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,704 pass(es).
[ View ]

Rerolled for Taxonomy becoming EntityNG

webchick’s picture

Title:Use type-hinting for entity-parameters» Change notice: Use type-hinting for entity-parameters
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Exxxxcellent!

Committed and pushed to 8.x. Thanks!

This is probably going to require some changes to some change notices, I suspect.

andypost’s picture

tim.plunkett’s picture

Berdir’s picture

Title:Change notice: Use type-hinting for entity-parameters» Use type-hinting for entity-parameters
Priority:Critical» Normal
Status:Needs review» Fixed

Looks good. Changed "must implement its own interface" to "should implement its own interface" as that's what you also used in the title and it's not a hard requirement. http://drupal.org/node/1982062/revisions/view/2667804/2667952

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

clarified issue summary

ianthomas_uk’s picture

Issue summary:View changes
Issue tags:-revisit before beta

revisit tag was added when this was postponed, but it's since been fixed, so tag is not needed.