[policy, no patch] Trait strategy

I'm not sure we should go that far, yet. Let's make the traits first and see what happens. If the base classes become just a collection of traits, then they may not have any further reason to live and can commit sepuku.

I'm marking this postponed and for revisting; let's circle back after we've had more trait practice. We need to get testbot up to speed first anyway, after which we can probably blitz through some trait usage.

I would be interested in taking a fresh look at ControllerBase, but FormBase is more than just a place to dump helpers. It extends DependencySerialization, it implements create() to be helpful, it overrides validateForm() since that is often not needed...

I'd like to leave FormBase out of this.

I don't see the value of this. Even if all ControllerBase becomes is a dumb wrapper for 39 traits, that's 39 fewer traits that all people writing modules with pages everywhere need to know about in order to perform basic tasks.

Generalizing the issue some based on discussion with amateescu, and reopening.

webchick: The value is that you really don't need 39 utility service wrappers. You need 3. (If you need 39, that's a bad sign that your code is already doing too much and should be broken up.) Using 3 traits in your class is not a high burden, and it also helps to identify what your dependencies are, too.

Added some possible traits to the list.

All abstract base classes SHOULD be considered as candidates to replace with one or more traits. (Reason: abstract classes exist only to provide incomplete partial implementations,

Disagree. An abstract base class still has a very different use case. They are the top of a logical hierarchy. A class like the abstract Entity base class should probably stay where it is.

Traits should be used when when a class needs to play a particular role that is ancillary to it's primary role or identity.

For example, this could be abstracted from ContentEntityBase:

namespace Drupal\Core\Entity;

trait Revisionable {

  public function setNewRevision($value = TRUE) {
    $this->newRevision = $value;
  }

  public function isNewRevision() {
    $info = $this->entityInfo();
    return $this->newRevision || (!empty($info['entity_keys']['revision']) && !$this->getRevisionId());
  }

  // etc
}

DependencySerialization should definitely be a trait...

Reworded the abstract class note to say that all abstracted classes are possibilities to be dissected for traits. Some may make sense, but all should be considered to see if traits would make more sense. Mark, does that work for you?

Added DependencySerialization, generator, and current user proposed traits.

Certainly better.

I would maybe say that it should be considered for abstract classes used purely for code sharing, as opposed to abstract classes that are the root of an inheritance hierarchy.

Let's compromise on this. (In practice I think a lot of "is a" relationships SHOULD be represented by interfaces, not base classes, but we probably don't have the time to switch to that fully.)

Traits and interfaces aren't mutually exclusive, we'd just use both. You still need the interface because you can't type hint traits.

Er. I never said they were. The pedant in me would argue that given interfaces and traits, you don't need abstract classes anymore. Interfaces and traits make a very powerful and complementary toolset. (One is reusable contract, the other reusable code.)

agree with @msonnabaum in #11 here - there are still legitimate cases for abstract classes. if a given hierarchy is mostly about implementing a single interface, then it is likely a better fit for an abstract base class than a trait. the simple fact that there can be only one parent, but multiple traits, can be used to elegantly differentiate the primary vs. ancillary responsibilities of a class.

the cases that are ripe for refactoring into traits are ones where a base class has been created that is either a) significantly incomplete or b) its subclasses consistently override identifiable subsets of its methods. if we're to go more theoretical, then it would be the extent to which Liskov is adhered to by the hierarchy.

i have some clear, good uses for traits over in #1762204: Introduce Assetic compatibility layer for core's internal handling of assets that i'll be converting soon. mostly of the "ancillary role" variety, but also one of the "significantly incomplete" variety. they might not be good exemplars, though, as that system has yet to be committed, it's complex, and people don't know it.

Infrastructure-level and domain-level classes (ie, things that we do want to unit test in the majority case) MUST NOT use traits as an alternative to proper dependency injection.

I don't understand this. What is meant by "proper dependency injection" here, and what makes using a trait improper? Do you mean "proper" = "constructor injection", and therefore, "improper" = "setter injection"? Per #2019055-187: Switch from field-level language fallback to entity-level language fallback, I disagree with that. And if setter injection is proper, then what's wrong with the setter being in a trait?

oh, yeah. echoing #20 - that also leaves me a tad confused. traits are perfectly unit testable on their own; could an example of the undesirable behavior be provided for clarification?

Also, adding that Traits are (like the rest of PHP's object architecture) a very static mechanism, and as a consequence not appropriate for dynamic horizontal extensibility. Drupal should not start using Traits for mechanisms that need to be added or removed from a particular class, or switched out to a different implementation on a site-by-site basis.

So from #11, Revisionable as a trait? No thank you. The data model layer should probably stay of as far away from Traits as possible, and implement other more configuration driven mechanism to achieve the desired code reuse.

Re #20: I mean that it's quite tempting to make a trait for any vaguely useful service and just suck that in instead of wiring something up in the DIC. It's not just constructor vs. setter injection; it's "don't even hard-code a default, make it a dependent service as it should be." Traits are not a replacement for composition and we should not think of them as such, no matter how tempting it may be to bypass DI with default traits. That creates silent, hard to find coupling that will just make our lives more painful in the long run.

Re #22: While in general I agree with you that Drupal's extremely heavy emphasis on runtime configuration of the domain model limits some things we'd do easily via code, I don't think that's the case for everything. Right now, whether or not an entity type supports revisions is a code-time decision. As a code time decision, traits are not unreasonable. If it became a config-time decision then I'd agree with you, but we still have many many code-time decisions in Drupal and always will. Using traits for those where appropriate is completely reasonable.

More on traits:

http://www.garfieldtech.com/blog/beyond-abstract

Added tag and issue summary template. Also added a task to the summary: if we adopt the use of Traits, we will need to make sure that api.drupal.org can handle them, which will mean updates to the Grammar Parser module as well as the API module code.

Added one more task (make a guildelines page) to summary and fixed HTML typo.

I assume the introduction of traits will be a significant API change, in which case it should be done before beta, yes?

Tagging as revisit before beta, but I think this should probably be a beta blocker, as should some of the issues that will come out of any decision (although they will be immediately postponed on PHP 5.4 testing infrastructure).

Yes, (proper) migration to PHP 5.4 is an inherent part of bumping requirements to PHP 5.4 in the first place. The topic of bumping requirements to PHP 5.4 is not limited to traits at all, so do not question that, please.

Therefore, coming up with a proper plan for when and where to use traits is a beta blocker.

We need more discussion around potential use-cases for traits — but at the same time, we need to draw a hard line on sensible/possible usages of traits within the context of 8.x.

In other words, we need a discussion on "How much code repetition pain is acceptable for D8?"

It would be great if we could agree that we should no use Traits for code like that in random classes:

class foo {

  public function getModuleHandler() {
    if (!isset($this->moduleHandler)) {
      $this->moduleHandler = \Drupal::moduleHandler();
    }
    return $this->moduleHandler;
  }

}

In constrast to the translation service this is a clear dependency and relavant for the logic of the object.

I'd go as far as saying that module handler should not be handled with that sort of wrapper method at all; if you want the module handler, inject it properly like everyone else.

Not convinced that trait usage means a serious bc break for implementing classes. If I've got that wrong please correct me, but changing to beta target until that's demonstrated.

In constrast to the translation service [moduleHandler] is a clear dependency and relavant for the logic of the object.

I'd go as far as saying that module handler should not be handled with that sort of wrapper method at all; if you want the module handler, inject it properly like everyone else.

I don't get what makes moduleHandler() different from translation service with respect to DI. For example, NodeController::add() calls on moduleHandler() in order to determine the langcode to initialize a new node to. How is that more relevant to the logic of a controller than translating a string?

Also, what about config(), which is listed in the issue summary as a good candidate for a trait (which I agree with)? What makes moduleHandler() more relevant to the logic of a controller than getting a configuration value?

What I think would be best for DX is for there to be a trait for every service that currently has a wrapper method in ControllerBase. Possibly other core services as well. Is there a reason we need to agree on the full set in this issue vs. discussing each one in its own issue?

catch: "Things we want to do with traits, once we have them" include changing how the various utility base classes work, and possibly eliminating some entirely. Unless we say that we're going to do all the work to make, eg, ControllerBase unnecessary and then keep it anyway, this is beta blocking. (We MAY end up keeping it; but we shouldn't force that simply for logistical reasons. It should be a technical decision.)

effulgentsia: I don't think we need a canonical list here. We need guidelines for what kinds of things should/shouldn't get a trait so that we don't end up with a definition of "these services had someone willing to sneak in a trait before anyone else noticed but those didn't", which is what the default will be if we don't have such guidelines. IMO, the guideline is "common application-level tools that are incidental to the operation of the object". Translating strings the way we do is incidental to the object's operation. It's just a Drupal thing. Invoking a hook, or an event, is not incidental to an object. It's part of the object's behavioral definition. That should be injected.

I don't want to say "all of ControllerBase" categorically since I think new stuff has slipped into that class while I wasn't looking, which is exactly what we don't want to happen. :-) (Not just me in particular, but ad hoc one off "seemed like a good idea at the time" does not a good architecture make.)

I agree that the plan ("guideline") is beta-blocking, because unless I'm missing something big, every single use of a trait will cause a move from constructor injection to setter injection, which presents a major API change in every single instance?

@effulgentsia's proposal to use ControllerBase as a baseline for this discussion makes some level of sense to me. At the same time I agree with @Crell that, perhaps, not everything in there ought to be a trait; needs analysis.

I could very well be wrong, but I'd assume that there should be traits for the "popular" base system services that make up the Drupal 8 application and which you typically need to get anything done without having to constructor-inject 10 dependencies (or resorting to inject the whole container); i.e., module/theme/extension handler, string translation, config.factory, database, this static thing called Cache, etc.

IMO, the guideline is "common application-level tools that are incidental to the operation of the object". Translating strings the way we do is incidental to the object's operation. It's just a Drupal thing. Invoking a hook, or an event, is not incidental to an object. It's part of the object's behavioral definition. That should be injected.

Per #32, I'm going with the assumption that config() will have a trait, so by this logic, we would put it in the "incidental" bucket. If that's true, then how is adding a flexibility point via invoking an alter() hook more "part of the object's behavioral definition" than making it flexible via a config() setting? Note, I'm not saying config and alter are the same thing, but I'm questioning how one is more or less incidental than the other.

I also somewhat question the "incidental" vs. "part of behavioral definition" as the metric as the primary one to focus on. The reason is because I think I disagree with the following:

Traits are not a replacement for composition and we should not think of them as such, no matter how tempting it may be to bypass DI with default traits. That creates silent, hard to find coupling that will just make our lives more painful in the long run.

I don't see traits as any more or less silent than all of our current create() methods. As an example, consider BanAdmin with the following code:

class BanAdmin extends FormBase {

  /**
   * @var \Drupal\ban\BanIpManager
   */
  protected $ipManager;

  /**
   * Constructs a new BanAdmin object.
   *
   * @param \Drupal\ban\BanIpManager $ip_manager
   */
  public function __construct(BanIpManager $ip_manager) {
    $this->ipManager = $ip_manager;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('ban.ip_manager')
    );

Now, I would argue that BanIpManager is unquestionably intrinsic to this form's behavior and not in any way incidental. However, with respect to silent couplings, I would argue that the following:

class BanAdmin extends FormBase {
  use BanIpManagerDependency;

is no more silently coupled than the former code. The former code is coupled to there being a container with a 'ban.ip_manger' service unless the calling code calls new directly rather than create(), and the latter code is coupled to there being a container with a 'ban.ip_manger' service unless the calling code calls a setter method. The latter code has the exact same coupling and conveys the exact same information about how the class can be instantiated (with only a change from constructor parameter to setter method); the only meaningful difference between the two approaches is that the latter has fewer lines of code in the class.

Based on this, I don't see trait vs. no trait as having any architecture impact on the classes/objects and their reusability. To me, it's purely a DX issue, for which I think we should evaluate:

1. Is it worth the bother of creating the trait at all? For example, in the above example, there might simply be not enough reuse of BanIpManagerDependency to be worth the separation.
2. Is there any code that we care about that constructs the object via new rather than via create()? In the case of controllers and forms, I think the answer is no, other than unit tests, and I don't think we care all that much about unit testing controllers and forms. In the case of other classes, we care more about unit testing them. And of course, there are many classes where non-test code instantiates via new, which we should care the most about.
3. For the code that we care about that instantiates via new, how much better DX is it for dependencies to be passed in the new statement vs. in a setter method? This is where I think the incidental vs. intrinsic metric can come into play, because IDE autocompletion puts constructor parameters more in your face than setter methods, so using constructor parameters for the "more relevant to the class behavior" dependencies and setter methods for the "less relevant to the class behavior" dependencies makes sense.

So to summarize, rather than having a guideline that's strictly based on incidental vs. intrinsic, I'd rather have a guideline that's based first on whether there's actual code that instantiates via new, and if there is, then deciding which dependencies are most helpful to bring attention to in the constructor.

Being in __construct() is about as non-silent as we can get. Regardless of the static factory method pattern we chose, that is how we make this explicit. Traits absolutely move us away from that.

Unless we say that we're going to do all the work to make, eg, ControllerBase unnecessary and then keep it anyway, this is beta blocking.

I'd need a lot of persuading to remove ControllerBase entirely at this point, that's likely to affect a very large number of contrib modules. If there ends up being a compelling reason to remove it, then we can bump this issue (or that one) to beta blocker, but I don't see anything here now. Also "using traits" isn't a release blocker for me: compared to all the other crap that needs fixing in core it is pretty minor.

So... Let's step back a moment.

What is the definition of Beta? At some point, we need the API for module/theme developers to be stable, and I thought/hoped that this point was defined to be the beta (barring fixing critical bugs necessitating further API changes). Actually I thought it was supposed to be long past, but our APIs are still changing a lot and I for one have given up trying to port D7 modules to 8 for the time being as being a waste of time, since I'll just have to do it over again.

Anyway, whenever that "it's time to port" point is, we would need stable base classes and interfaces for module developers to use when making their own plugins, entity types, etc., and we wouldn't want any further developer-useful base classes or interfaces to be changing after that point, in ways that would require the module developers to change their code in any way, right?

So will adopting Traits for a particular class/interface change or eliminate developer-facing classes and interfaces? The issue summary seems to imply that it will. If so, then it seems like the Traits adoption needs to be actually done (not just the strategy agreed upon) pre-beta. If they don't affect the developer-facing API, but are just "internals", then who cares: we can convert classes to using traits even after 8.0 is out.

And if I'm wrong about beta being "the API is stable" point, then ... when is that point, and when can module developers really get to work with the expectation that they can do their port and expect it to still probably work when 8.0 comes out?

Being in __construct() is about as non-silent as we can get. Regardless of the static factory method pattern we chose, that is how we make this explicit. Traits absolutely move us away from that.

I'd like to understand what your definition of non-silent and explicit is. If we name our traits with some convention (e.g., BlahBlahDependency), how is a use XDependency, YDependency, ZDependency, ... line at the top of the class definition more silent or less explicit than constructor parameters? I'd agree that it's perhaps less prominent, which is afterall, the point: to remove noise from the code so that it's easier to see and focus on what the class does. But that doesn't make it silent or non-explicit.

What is the definition of Beta? At some point, we need the API for module/theme developers to be stable, and I thought/hoped that this point was defined to be the beta (barring fixing critical bugs necessitating further API changes).

Traits in most cases will be API additions, not changes.

Where using traits in core would result in an API change, we'll need to evaluate that case by case for impact. However I don't have any interest in blocking beta (and hence the release as a whole) on using traits - if we end up with nice to have trait usages we can't do because they're API changes and too late, then that'll be like any other API change that's too late.

Here's a way we can make traits completely non-API-breaking: decouple them from being opinionated on setter vs. constructor injection.

In other words, for each core service that is a common enough dependency to other classes, create a trait whose implementation is a protected getter and setter method. Then, for each class that uses that trait, it can choose one of three things:

1) Use constructor injection by adding the dependency to the __construct() signature, and within __construct(), invoke the protected setter method.
2) Use setter injection by changing the setter method visibility to public within the use statement.
3) Do neither, in which case the dependency can't be injected, and is hard-coded to come from the container. Core shouldn't use this option, but if contrib does (by not explicitly doing either of the above), it's not the end of the world. If someone wants to reuse that contrib class and add injection to it, they can extend the class and do either of the above in that subclass.

So with this, it wouldn't be traits that change API. For example, we could add a trait for every service exposed in ControllerBase, make all core controllers use the traits they need directly rather than extending ControllerBase, but for contrib BC, leave ControllerBase around as a class that uses those traits but has no other implementation code.

But, what would be an API change is changing a class from constructor injection to setter injection or vice versa, so whatever we want to change in that regard after beta, a core maintainer will need to evaluate in terms of whether the constructor signature of that class is too relied on by contrib and no longer worth breaking.

Does this sound about right, or am I missing something?

effulgentsia: What I don't get is why setter injection is even being mentioned in this thread. What has that to do with anything?

From a cleanliness/purity standpoint, the ideal is every class gets every dependency is has via constructor injection. End of story. Using the create() method doesn't impact that.

However, we have a number of super-common dependencies that are annoying to have to do manually each time. Translation, link generation, maybe config, that sort of thing. For those, we *currently* are using a common pattern in base classes to pull them directly from the DIC via \Drupal, with setter methods *for testing only*.

Nowhere in our current approach are we implying setter injection in the typical case.

The proposal with traits, and what instigated this thread, is: Utility base classes suck, because they imply an is-a relationship that doesn't exist. They're abusing interface-reuse for code-reuse, which is a bad thing. See http://www.garfieldtech.com/blog/beyond-abstract

So we should *not* be using traits as an alternative to composition. They are not an alternative to composition. We want to use them as a less hacky way to simplify composition of selected objects that are annoyingly common, but still encourage people to use constructor injection for most non-incidental services, because that's the cleaner approach. As Tim said, that's about as explicit as you can get.

Even naming the traits *Dependency is, I think, a misnomer. They're container-aware utility methods that we're making it easy to copy-paste. And I at least very much do *not* want to encourage people to use that approach for anything other than those selected services that are common application level Drupal things. We have an entire community we want to teach good development practices to by good example. Don't start by making sloppy easy.

What I don't get is why setter injection is even being mentioned in this thread.

Cause the patch in #2079797: Provide a trait for $this->t() and $this->formatPlural() has a public setter. However, after doing some googling and reading articles about the flaws of setter injection, I'm won over to not doing it.

we *currently* are using a common pattern in base classes to pull them directly from the DIC via \Drupal, with setter methods *for testing only*. Nowhere in our current approach are we implying setter injection in the typical case.

Traits used for common application-level dependency injection (like translation) MUST include an override method for easier testing.

ControllerBase does not currently have public setter methods. So if we want to discourage setter injection (and like I said, I'm won over to that), then why add public setter methods to the traits? Instead, what if we just have a protected variable for the override, and require tests to either subclass with a longer constructor or inject a mock container in order to override? Neither is that cumbersome to do, and that would remove the various problems of people invoking setters at inappropriate times in real code.

However, we have a number of super-common dependencies that are annoying to have to do manually each time.

Here's a list I found by grepping for existing non-test classes that invoke Drupal:: currently in HEAD. This isn't the full list, but is the ones that stood out the most to me:

request()
currentUser()
entityManager()
entityQuery()
cache()
config()
state()
moduleHandler()
urlGenerator() / url()
linkGenerator() / l()
translation() / t()
languageManager()
formBuilder()

Can we agree that all of these warrant a trait? #29 argues no for moduleHandler(). Which others are disputed?

but still encourage people to use constructor injection for most non-incidental services

Can we have a situation where one of the above services is incidental to some classes, but not incidental to others? Related to that, the issue summary says:

Infrastructure-level and domain-level classes (ie, things that we do want to unit test in the majority case) MUST NOT use traits as an alternative to proper dependency injection. ONLY application-level code (controllers, forms, etc.) should do that.

I dislike the thought of some controllers having code like $this->t() and $this->state()->get() while other controllers and domain classes have code like $this->translationManager->translate() and $this->state->get(). Would it be ok to have all classes that use a service for which we have a trait to use the trait and its getters / helper wrappers, and also add the dependency to the constructor where proper DI is needed?

The setters on the traits are strictly for testing purposes. If we're going to be veering off of the ideal path with "service traits" in the first place, IMO a setter on those for testing purposes is better than "you can't test it unless you subclass it"; if nothing else it's less work, and we want testing to be easy. If it's not easy, people won't do it.

As far as infrastructure/domain classes, I think all controllers (ie, page controllers, the things called by the kernel) count as application-level. We can't really stop someone from constructor-injecting the translation system into their controller, but I wouldn't expect them to do so. The same is probably true of most Plugins, for instance.

If, however, someone were to try and use TranslationServiceTraitThingie in, say, the Entity system, or some class in routing, or in the UrlGenerator itself, or godforbid in the database system, that's where I would get right cross. Those are cases where we should "do it right", because it's not an application-level object that thousands of people will be writing thousands of. Those are plumbing of the system, and we want the plumbing to be as clean as possible so it doesn't get clogged. &lt,/gross-metaphor>

If, however, someone were to try and use TranslationServiceTraitThingie in, say, the Entity system, or some class in routing, or in the UrlGenerator itself, or godforbid in the database system, that's where I would get right cross.

If we look at the StringTranslationTrait in #2079797: Provide a trait for $this->t() and $this->formatPlural(), it does 4 things:

• defines a protected variable.
• has a protected getter method for that variable, with a fallback.
• has a protected helper method to reduce the verbosity of invoking the most common method on that variable.
• has a public setter method to allow unit tests to set that variable, and we hope that developers will refrain from invoking it outside of unit tests.

I'd like to suggest that we decouple those first 3 from the last one, consider those first 3 as what's most important about the trait, and then consider how to optimize unit tests without adding a public method that we then need to instruct people to not use outside of tests.

Within search module, we current have:

• Drupal\search\SearchPageRepository calls $this->configFactory->get('search.settings')->....
• Drupal\search\SearchQuery (not a plugin) and Drupal\search\Plugin\views\filter\Search (a plugin) call \Drupal::config('search.settings')->....
• If there were a controller or form that needed that config, it would call $this->config('search.settings')->....

Similarly, within node module, we current have:

• Drupal\node\Entity\Node (an entity class) calls \Drupal::entityManager()->... and \Drupal::moduleHandler()->....
• Drupal\node\Plugin\Search\NodeSearch (a plugin) calls $this->entityManager->... and $this->moduleHandler->....
• Drupal\node\Controller\NodeController (a route controller) calls $this->entityManager()->... and $this->moduleHandler()->....

I think the subtle inconsistencies in the above is bad DX, and I think traits give us the opportunity to standardize across the board on the last variant of each of the above. Especially, if the trait getter were written like:

protected function entityManager() {
  if (!$this->entityManager) {
    // @todo Bikeshed the method name "getDrupalService".
    $this->entityManager = $this->getDrupalService('entity.manager');
  }
  return $this->entityManager;
}

Then, within classes where we want to require 100% DI with no possibility of a fallback, the getDrupalService() method is undefined and will correctly trigger a fatal (making @Crell happy, I hope). Or we can have a trait that implements that method to throw a friendly exception. For classes where we want the fallback to be what's in the Drupal container, we can have a separate trait that implements that method as protected function getDrupalService($id) {return \Drupal::service($id);}, and for classes where we want the fallback to be what's in an injected container, we can have a separate trait that implements it as protected function getDrupalService($id) {return $this->container->get($id);}. I actually suspect that there's no good use case for that middle option, and that an injected container would always be preferable, but I added it here for completeness, since that's what's in #2079797: Provide a trait for $this->t() and $this->formatPlural().

In summary, traits allow us to call $this->t(), $this->l(), $this->config(), etc., everywhere, consistently, regardless of whether we're in a domain class with 100% proper DI or a controller with a service locator fallback.

If we're going to be veering off of the ideal path with "service traits" in the first place, IMO a setter on those for testing purposes is better than "you can't test it unless you subclass it"; if nothing else it's less work, and we want testing to be easy. If it's not easy, people won't do it.

What I dislike about adding a public setter to the trait is:

• We then can't use the trait within an infrastructure/domain class, because we corrupt a properly constructor injected class with a public setter that can be called at any time, leading to all sorts of brittleness. As a result, we're left with the inconsistency of $this->t() in some places and $this->translationManager->translate() in others.
• Even in controllers and such, a public setter shows up in IDE autocompletion, etc. A public method we ask people to not invoke is a code smell.

Meanwhile, people working with PHPUnit are already used to the idea of mock classes, where getMock() returns an object of a dynamic class that emulates in some ways the class that you care about. So, I don't think it would be hard for us to add a similar kind of utility method to our testing framework for creating a test double that's an instance of a dynamic class that's a subclass of the one you want to test. For example, in your unit tests instead of calling:

$controller = new SomeController($foo, $bar);
$controller->setTranslationManager($mock_translation_manager);

you would instead call something like:

$controller = SomeDrupalUtility::getTestDouble('\\Drupal\\some_module\\SomeController', array($foo, $bar), array(
  'string_translation' => $mock_translation_manager,
));

Not really that much more work or loss of clarity in the unit test, and IMO, the benefit of not needing a public setter polluting real code and making it brittle hugely outweighs the slightly more verbose way of instantiating a controller within a unit test.

As far as I see it the recent comments do not reflect Drupal 8 reality as we currently have it. To be perfectly clear, I want to completely avoid the discussion over constructor injection vs. setter injection, as the pros and cons in terms of theoretical purity are irrelevant for this issue, to my mind.

Here are a few things that I would consider facts, but let's see whether there is actually consensus/awareness of them:

1. We have a number of base classes that we encourage people to subclass from.*
2. Because subclassing a class with a large number of constructor injected services is a pain, we have moved away from constructor injection for base classes. Instead of setter injection, we have used the service locator pattern of retrieving services directly from the \Drupal class.
3. In almost (?) all cases, however, we need the respective setter anyway, for unit testing. This is currently done mostly by creating test-specific override classes that provide the setter method.
4. We want to replace the boilerplate copy-pasted code that each of these base classes have with a trait for each service.

Can we agree that the above is true?

If so, then I have the following question:
Limiting this strictly to the above use-case of a convenience base class, i.e. ControllerBase, is there any practical (!) reason, not to include the setter in the trait?

As I see it, it has only benefits:
* We can provide optional setter injection for people that want it.
* We don't need the test overrides, which are not horrible, but also not exactly pretty
* We get all that with 0 additional copy-pasted code.

Note that "it wouldn't be used often" is not a very good argument as we're already tainting our base classes with so many methods that any one subclass will very likely not use all of them and we're already providing $this->translationManager() or $this->configFactory(), for example, where most code uses $this->t() and $this->config() directly.

Again, I would love to know what specifically is made worse by providing the setX() method in the trait, vs. not providing it. I have re-read (almost) the entire issue here, but still come up short.

*I am not aware of any immediate plans to get rid of those so while their right to existence may or may not be undisputed, this fact should be considered immutable for the purpose of this issue, as far as I see.

Edit: fix markup

In almost (?) all cases, however, we need the respective setter anyway, for unit testing. This is currently done mostly by creating test-specific override classes that provide the setter method.

What's an example where we have test-specific overrides? I'd like to demonstrate how we can make those simpler based on #46.

is there any practical (!) reason, not to include the setter in the trait?

When I looked at ControllerBase in HEAD, I saw no setters, and given #43, I was concerned about us using this issue to create a new policy to add them, especially when I think they're unnecessary, per #46. However, when I look at other base classes in HEAD (e.g., FormBase and EntityControllerBase), then I already see pre-existing setters, so given that, I'm not opposed to maintaining that status quo in #2079797: Provide a trait for $this->t() and $this->formatPlural(), though I'd like some issue in which to argue for changing our code to not need them. Happy to do that either here or in a new issue based on what folks here prefer.

The point I care about more though from #46 is that I think if we have traits for things like translationManager and configFactory, then I think we should use those traits on all classes (at least within modules) that need access to them, so that those services are always accessed consistently with the same getter method. And I tried to argue in that comment that using a trait for its protected variable definition, getter method, and helper methods is in no way incompatible with DI purity / constructor injection. If for now, we have a public setter method on the trait, but don't want that public setter leaking into a class that receives the dependency in the constructor, then we can import the setter as private at the point we use the trait (e.g., use StringTranslationTrait { setTranslationManager as private }.

I don't think we need to hold up #2079797: Provide a trait for $this->t() and $this->formatPlural() on reaching consensus on whether to use it in more places though. However, since this issue is about our general trait strategy, I'd like to keep trying to convince people here of the merits of that (or be convinced of its problems).

One small point we might should take into account is that once we start relying on traits everywhere we cannot easy write good decoupled components anymore, as everything will be coupled with your Trait.

Note: One reason why the translation example is particulary bad is that for l10n.drupal.org reasons we need to add a dedicated method for t()/formatPlural(). For
generic components like our services, at least in /core/lib, most of the time don't deal with concrete strings but rather just variables in which case calling the translation manager is fine.

Good point. By "everywhere", I'm specifically interested in modules. In other words, I'd prefer all classes within node module to access the entity manager via $this->entityManager(), regardless whether the class is an entity type, plugin, entity controller/handler, route controller, query extender, etc., rather than sometimes as $this->entityManager, sometimes as \Drupal::entityManager(), etc.

For core/lib/Drupal/Component, I don't think we'll even have any traits there at all, right?

For core/lib/Drupal/Core, I'm not sure what's best. I think there's some benefit for any code there that relies on an entity manager to also access it as $this->entityManager(). And if a core component is already coupled to EntityManagerInterface, is there any actual issue with it also being coupled to a trait that's in the same namespace? OTOH, if there is an actual issue with that, then I think that would be an ok and grokkable reason to have core/lib and core/modules differ.

I would really appreciate it if someone would (seriously) correct me where I'm wrong in my train of thoughts:

1. The vast majority of our current services are the exact anti-definition of dependency injection: They require the whole DI container to be injected or alternatively passed into Drupalism construct of ::create(Container $container).

   Both are technically the same: A massive dependency on global state. If that was the goal, then we could have skipped the topic of DI entirely.

   This affects almost all controllers, almost all plugins, and a whole bunch of Core services. I personally notice every single day, because this fact causes debug_backtrace() to be dysfunctional, and that is an excellent benchmark for the topic at hand here.

2. I fully agree that (1) nothing in Component should rely on traits to function and (2) ideally, the majority of Core should not need traits either.

   That is, unless traits are used independently for the sole purpose to reduce unnecessary code duplication within a component, transparent to the outside world (unless the outside world re-implements a component service).

3. But the way I see it, the whole point of this issue and discussion focuses on user-land integration code for the application framework.

   I.e., we are talking about modules only, and within modules, we are mainly talking about controllers, entity/field/data handlers, forms, plugins, etc.

   Proper services within modules are out of scope already, because those follow the rules of core services; i.e., to be implemented in the most sensible way possible like any other service, which normally means constructor injection of all dependencies. (But to be clear, core has no business in making any decision about that anyway.)

   We consider to resolve two discrete aspects with traits: (1) Focusing on "actual" dependencies in constructors of integration classes instead of common application framework utilities. (2) But yet, still inject common utility dependencies into integration code (so as to e.g. react to a change in dependencies).

   This is my concrete understanding of the scope of this discussion.

4. Traits do appear to have the potential to fully eliminate the problem of 1), unless I'm missing something big, and again, this is the meat of where I could possibly be missing something:

   Traits essentially allow you to "attach" a common/standardized subset of methods to a class. Considering a theoretical model like synchronized services, the following is very well technically possible:

   1. Every trait maps to a "common application framework utility" service. A trait exclusively is a peripheral dependency to the integration code at hand.
   2. Upon instantiation of a service, follow the concept of what a synchronized service would do: setStringTranslation(), setModuleHandler(), setConfigFactory(), etc.pp.

   → All dependencies are actually injected, instead of being consumed from an injected container.

   → debug_backtrace() is happy again. And unit tests are very happy, too.

   Mayhaps a new pattern, but definitely better than the current ::create() anti-pattern. And also, common OO and Symfony constructs did not really help us further. We're here to solve an application framework challenge in a modular system that no one else appears to have solved yet.

Please, for the sake of world peace and making progress, I'm going to state a third time that my interpretation can possibly be wrong. Please correct me where I am technically wrong and please state why you think that is. — I'd like to stress on that aspect, because thus far, this entire discussion went this way: (1) Elaborative thoughts/proposals. (2) ~50 chars replies essentially just saying "no, thanks, you're Doing it Wrong™." — In order to get on the same page and move forward, we need collaborative and productive communication. Thanks! :-)

#51.1:
We never ever use the static factory method for actual services (those defined in *.services.yml). Those have their constructor args wired up in the YAML. And those are what @dawehner has asked us to never use traits for: true services that reside in the DIC.
Controllers, forms, plugins, title callbacks, route callbacks, and "entity controllers" are the only things that use a static factory method.

#51.2:
Agreed on Component never using traits, and Core only using traits within their shared \Drupal\Core\* namespace.

No real opinion on points 3 and 4 yet, but I think those are the ones we're discussing: modules.

The vast majority of our current services are the exact anti-definition of dependency injection: They require the whole DI container to be injected or alternatively passed into Drupalism construct of ::create(Container $container).

Both are technically the same: A massive dependency on global state. If that was the goal, then we could have skipped the topic of DI entirely.

This is not correct. All of these classes use constructor injection and are as unit-testable as anything in a *.services.yml. They do *not* depend on the container, their factories do. It just happens that their factories are static methods. This is a very important distinction. Also, not really a drupalism, we certainly didnt invent factory methods.

This is not correct. All of these classes use constructor injection and are as unit-testable as anything in a *.services.yml. They do *not* depend on the container, their factories do. It just happens that their factories are static methods. This is a very important distinction. Also, not really a drupalism, we certainly didnt invent factory methods.

+1. You could consider this create() factories as pure implementation detail. From an outer perspective you know the dependencies of your code in the constructor, which itself helps you to understand
quite a lot what the code could do.

Proper services within modules are out of scope already, because those follow the rules of core services; i.e., to be implemented in the most sensible way possible like any other service, which normally means constructor injection of all dependencies. (But to be clear, core has no business in making any decision about that anyway.)

Sadly we currently just talk about Trait as alternative approach for DI, while Traits are potentially useful for many other things.

Just a random thought:
From my perspective one reason why the DX of DI is seen as bad is often triggered by the verbosity of our code, here is one example:

class Plant {

  /**
   * The co2 service.
   *
   * @var \Chemistry\Molecules\CarbonDioxidInterface
   */
  protected co2;

  /**
   * The water service.
   *
   * @var \Chemistry\Molecules\WaterInterface
   */
  protected $water;

  /**
   * Constructs a new Planet.
   *
   * @param \Chemistry\Molecules\CarbonDioxidInterface $co2
   *   The co2 service.
   * @param \Chemistry\Molecules\WaterInterface $h2o
   *   The water service.
   */
  public function __construct(CarbonDioxidInterface $co2, WaterInterface $h2o) {
    $this->co2 = $co2;
    $this->water = $water;
  }

}

If you use proper tools and naming you would not need more than the following,
so modules, especially custom ones, will be able to just use the following.

class Plant {

  // The @var don't give you any information which cannot be derifed from static code
  // analysis of the constructor as Storm does for example, but sure you could just
  //  typehint them and don't do more.

  // The @param for the constructor also are kinda pointless. Your code should be written
  // against the interface, so we don't have to explain what this interface does here.
  public function __construct(CarbonDioxidInterface $co2, WaterInterface $h2o) {
    $this->co2 = $co2;
    $this->water = $water;
  }


  public function getOxygen() { ... }

}

Just one more thought that I realized discussing this with @dawehner:

having the following code:

public function getX() {
  if (!isset($this->x) {
    $this->x = \Drupal::x();
  }
  return $this->x;
}

public function setX($x) {
  $this->x = $x;
}

doesn't actually *prevent* constructor injection. You can inject $this->x from the constructor and just never call setX() and then \Drupal::x() will never be called and all is well. So by following this pattern we actually still give people the choice of how they want to inject stuff.

I think the confusion here is that there's 2 different goals being sought, by different people.

1) Simplifying application-level/integration-level code: There's a half-dozen or so services that are super-common and really annoying to inject manually all the time: Translation, config, state, link, etc. So make the common cases easier with a trait so it's one line instead of 10. That's especially useful in non-service classes that are using the ContainerInjectionInterface::create() method with a parent class, as you then have to deal with the parent's dependencies as well as your own. (These are the ones I'd like to largely dismantle.) But then we need to make it testable, too, which is where the setter method comes into play.

2) Common DX of invoking common services. That is, once you have, say, the link generator service injected into your object there's nothing inherently forcing you to use $this->generator->generate(), $this->generator()->generate(), $this->generate(), or $this->makeLink(). The code works either way. However, it's better for DX if the code "looks the same" regardless of the class in question. There is a meaningful difference here for translation, since the string extractor needs to know what to look for, but for all other cases I'd argue this is mostly an aesthetic question. This also applies to *all* types of classes, not just to "top level" application classes.

I'm focused mainly on the first. effulgentsia, it seems, is focused on the second, or on both of them together. (I don't want to speak for him too much.) To me, though, I honestly don't care about the second. Really. I'd rather force clean injection, even if it's more verbose, in lower-level services rather than scatter calls to to the container around the place; even if it's overridable, that's still service location, not DI, and the trend in the PHP world is very clearly (from the interactions I've had) toward DI over SL.

While it's possible to accomplish both goals with the same code (a trait something like #56), there is also a cost. Specifically:

1) Lots of service location instead of DI. Let's not fall short on this front. SL is a very easy trap to fall into, but it is a trap. Especially at lower-levels, we should avoid cutting corners.

2) It encourages people to use traits instead of constructor injection, because once you build the trait it is "easier". But this is just a repeat of point 1: It encourages people to couple to the container, and I guarantee you people *will* "do it wrong" and not allow for proper overrides.

3) Stealth dependencies. It becomes much harder to track what services depend on what. With all constructor injection, the container itself (or the services.yaml files) form their own dependency graph for us, or nearly so. If we have a lot of dependencies that are hidden behind a trait, as a service-locator, it becomes much harder to know what a class's dependencies are, how tightly or loosely coupled our code is, or how much would break if we made a given change.

It would be possible to separate those two goals into separate traits (one for the common utility method, one for the service-locator code) and require people to use one or the other or both, but I suspect that's going to get quite confusing quite fast.

My recommendation (pleading?) then is simple: Let's deal with just point 1 for now. If we really really care about point 2 we can do it manually, but I really don't think it's worth bothering with. Let's not try to squeeze that in as well when there are plenty of downsides (as above).

Thanks @Crell for that write-up. I agree completely.

What I don't understand, though, is where the discussion on DI vs. SL comes in in the first place. I.e. why is that part of this issue, why is that relevant to us introducing traits? It is a fact that we're currently using SL for base classes. So why are we discussing DI and more specifically CI when discussing how to make traits out of those base classes?

To frame the question more concrete: How does the possibility of using traits make CI easier/better/??? for base classes?

My recommendation (pleading?) then is simple: Let's deal with just point 1 for now.

With the "for now" caveat, I'm ok with that. Just a heads up though: once we have traits for what we agree on as the set of super-common services, I will open an issue to argue for point 2. When I do that, I'll add a link to it here, so folks here have a proper chance to give feedback on it. Perhaps with a real patch, it will be easier to debate the pros/cons than in the abstract.

So why are we discussing DI and more specifically CI when discussing how to make traits out of those base classes?

Apologies for my contribution to that. I saw this issue's title as trying to settle on something broader than just "make traits for (some of) the services already being accessed via SL from within existing base classes, and only use them within classes that would otherwise extend from those existing base classes". Particularly, I was objecting to this part of the guideline in the issue summary:

Infrastructure-level and domain-level classes (ie, things that we do want to unit test in the majority case) MUST NOT use traits as an alternative to proper dependency injection. ONLY application-level code (controllers, forms, etc.) should do that. (They may still use other traits as appropriate, but not as an alternative to dependency injection.)

I tried to argue that the same traits could be used by domain-level classes within modules (e.g., within Node.php) without it being an alternative to DI, but per above, I'll take that up in a new issue. (Side note: all of our entity types are already failing to use DI, even without traits.)

So, then, what is the scope of this issue? Do we still need to decide on which subset of the services in #44 we want to make traits for? And if there are some services in our current controller/form/plugin base classes that don't make the cut, then do we also need to change those into being constructor injected rather than service located?

#51.3:
I'm still assuming that entity types, field types, and some other plugin types (e.g., image effects? text filters?) are "domain-level", so as the issue summary is currently written, even ones that are module-provided would not be allowed to use the traits covered by this issue. If I'm misunderstanding what is meant by "domain-level", then please correct me.

#51.4:
create() is there to handle the actual dependencies, not the peripherals. Are you suggesting though that we remove the \Drupal:: fallback from the traits, and instead require whatever framework code calls create() to then also call the various peripheral setters? If so, do you want that explored in this issue, or as a follow up?

Re #55, @dawehner: #2209735: [policy] Remove DI verbosity by removing unnecessary doc lines

Thank you!

(Side note: all of our entity types are already failing to use DI, even without traits.)

That's an existing bug, and is a bug regardless of whether there are traits involved.

So what all are the services we want to do just part-1 to? Anything beyond what's in the summary now? I'd suggest adding the state system to it and then calling it a day. (I was just working on a sample D8 module and doing a bunch with State API.) We need to get this issue closed and get on to implementing things.

So where are we regarding setter methods?
This part of the issue summary could be interpreted as suggesting to use setter methods, but above it doesn't seem as though there is consensus around that:

Traits used for common application-level dependency injection (like translation) MUST include an override method for easier testing.

I understand the agreement to be that the trait include a public setter method, but that no core (and contrib?) code use it outside of unit tests. If that's correct, do we want to add a leading "_" to the method name and annotate/document it as @internal, or something like that?

@internal makes sense to me, possibly with other such notation/warnings in the docblock.

@internal Yet another tag... :( There are people already complaining that our docs standards are complicated enough without adding another tag to be used. Just saying...

It's new to us, but it's pretty standard: http://phpdoc.org/docs/latest/references/phpdoc/tags/internal.html

not having an interface is enough imo.
doing method_exists() all over the place to avoid construction injection, kinda defeats the purpose

Agreed: @internal is new to us and it is standard.

It's just that I keep hearing complaints about having more things added to the docs standards, and so I'd be reluctant to agree to the idea of "If a method is for internal use, you must add @internal to its docs header". If we make it a "You could add @internal", then it's kind of pointless (i.e. not a standard).

not having an interface is enough imo.
doing method_exists() all over the place to avoid construction injection, kinda defeats the purpose

Well, it's the class itself that decides what to add to its constructor signature, so the consumer of the object isn't choosing between method_exists() or constructor injection; it's choosing between overriding or not. For example, is it ok for a contrib module to do something like:

function mymodule_form_FOO_alter(&$form, &$form_state) {
  if (isset($form_state['build_info']['callback_object']) && method_exists($form_state['build_info']['callback_object'], 'setConfigFactory')) {
    $form_state['build_info']['callback_object']->setConfigFactory($custom_config_factory);
  }
}

Note that this has all the problems of setter injection / mutability in that a different config factory was used for all of the form's ::config() calls prior to the alter() running, and the new one is used for all calls after that.

This is what HEAD currently allows (FormBase has a public setConfigFactory() that isn't documented as internal), but if we believe this to be bad API design, then let's communicate clearly that the setter isn't for external use.

So what all are the services we want to do just part-1 to? Anything beyond what's in the summary now? I'd suggest adding the state system to it and then calling it a day.

I agree with adding state().

Question: Issue summary currently lists UrlGenerator, but not LinkGenerator. Do we want both in a single trait, separate traits, or not have a trait for LinkGenerator? Current HEAD has just as much usage of linkGenerator() as urlGenerator().

From what else is in ControllerBase:
- cache(), keyvalue(), and languageManager() don't have much usage, so I think we can skip on making traits for them and require constructor injection for them.
- moduleHandler() has a lot of usage though, so I'd like a trait for it, or to understand why it shouldn't have one.
- formBuilder() has little usage, but entityFormBuilder() has a lot. What are people's thoughts on these?

And from FormBase, we have getRequest() which has a lot of usage. Is this an ok one for a trait or no?

moduleHandler: Does it have a lot of usage, or a lot of usage *in controllers, forms, and plugins*? Those are the types of objects that we're targeting for the SL traits, which I anticipate people will be writing the most. I doubt many of those are invoking hooks on a regular basis, and if they are that's probably a bad sign.

entityFormBuilder: Same question. How many application-level classes are using it, and why? (Really, why? Entity Manager I can understand, but form builder?)

getRequest(): Non-issue for controllers as they can already get the request by specifying it in the method signature. The same for forms. That leaves plugins/blocks. For those, I'm inclined to say we *don't* want it, for the simple reason that we want to discourage directly request-dependent code. Really, we need less of that. Don't make it one-liner easy to do something that you shouldn't be doing. :-)

moduleHandler():
Having the whole module handler available is pretty bad, because it has knowledge of all of Drupal in there. Ideally it would just be used for invokeAll() and alter(), which would be events instead...

entityFormBuilder():
In routes, _entity_form doesn't work for add/create forms, since each entity type has different needs when being created. So with few exceptions, we have one call to entityFormBuilder() per entity type, in the controller for add/create. I don't think that's trait worthy, but I still don't have a better idea.

getRequest():
For controllers, it can be added to the method easily. But for forms, it's not quite the same. You have to put it in the buildForm() method, which is on an interface, so it must be Request $request = NULL. And then you have to be sure to pass it up through the parent::buildForm, and it all got very messy and was really pretty awful. But I'm not as worried about forms, since FormBase is nice to use for validateForm(), setFormError(), and possibly a future getFormId() default anyway.

moduleHandler():
Having the whole module handler available is pretty bad, because it has knowledge of all of Drupal in there. Ideally it would just be used for invokeAll() and alter(), which would be events instead...

That's a problem with moduleHandler() though and was known when it was originally commit. I can't find it, but there's an issue to split it into hook invocation vs. module CRUD (and whatever else is in there right now), in which case it should have considerably less dependencies.

I can't find it, but there's an issue to split it into hook invocation vs. module CRUD (and whatever else is in there right now), in which case it should have considerably less dependencies.

It's an old one, but #2004784: Move module install/uninstall implementations into ModuleInstaller. Alternatively, do we want a facade class that only implements invokeAll() and alter() (and moduleExists()?), and would that be ok to have a trait for?

I doubt many [controllers/forms/plugins] are invoking hooks on a regular basis, and if they are that's probably a bad sign.

There are still quite a few in core, and I'm sure in contrib. If we believe it's a bad practice, do we need issues to remove the instances in core (i.e., move the hook invocations (and also moduleExists() checks) from the controller to a domain or service object)?

We probably should, but IMO that's not release-blocking right now nor is it a blocker here; for now we just need to agree that we're not going to make a trait for it and move on.

I can agree to not making a trait for moduleHandler(), but I don't agree to adding $module_handler to the constructor of a dozen core controllers. Which means we need to leave it in ControllerBase until such time as we refactor those controllers. However, if we leave it in ControllerBase until after beta, then contrib modules will write some controllers that extend ControllerBase and have $this->moduleHandler()->... code in them, and those contrib authors might be annoyed if we then remove moduleHandler() from ControllerBase after beta.

Re #74, I have the same concern about entityFormBuilder(). I'm ok with not having a trait for it, but I'm not ok with removing it from ControllerBase so long as there's many core controllers that use it, but I'm also concerned about letting it sit past beta and removing it after.

Leaving getRequest() in FormBase makes a lot of sense, even past beta and release, so I think we're good there.

In #2208115-10: Add DependencySerializationTrait @sun mentions an interesting idea, that might solve (or help solve) the dependency serialization mess and is relevant to the getter/setter discussion here.

If - instead of getX() and setX() - we have a single x() method (i.e. moduleHandler()) which looks like the following:

public function x(XInterface $x = NULL) {
  static $instance;
  if (isset($x)) {
    $instance = $x;
  }
  return $instance;
}

Then injected services are not stored on the object and thus are not serialized.
As mentioned above, this would not in any way hinder constructor injection:

public function __construct(XInterface $x) {
  $this->x($x);
}

To *prevent* setter injection we could simply make the x() method protected. In this setting public vs. protected is equivalent to the setter vs. no-setter discussion above. I.e. I would be in favor of making it public and it seems there is tentative consensus around that.

some more issues are adding traits. I think tagging issues with "Traits" would be a good idea.

Wile working on #2225759: Derivative, FieldType, and BlockManager classes should implement the method t() I have a need for guidance on when to use a trait or not. I've read parts of this issue, but jargon and contradicting opinions make it difficult for me to extract a simple guideline. The documentation on traits (ref. in issue summary) does not yet exist. But with 18 traits in core now, I am probably not the only developer who is looking for simple and clear rules when to use a trait.

Sutharsan: I still stand by my explanation here as my recommendation: http://www.garfieldtech.com/blog/beyond-abstract

Don't use them as an alternative to dependency injection, 98% of the time. Use them as an alternative to inheritance when there isn't a very very clear "is a special case of" relationship.

Use them as an alternative to inheritance when there isn't a very very clear "is a special case of" relationship.

I think, instruction like this are very hard to digest. To be honest, it does not help me. We also need the easy explanation as in the TranslationTrait change record: "If your class is neither a controller or form, you can use the StringTranslationTrait." or explain it using examples.

Will read your article tomorrow, not today (Yaaarrrrr ;) )

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

I've moved the issue to 8.2.x since adopting new coding standards is considered a disruptive change, but it might be possible to add traits with BC in a minor.

I disagree with the notion that traits are generally "cleaner" than abstract base classes.
Or rather, it depends very much on the use case.

Maybe I am not saying anything new here, I just want to make sure we are not going too far with this.

Traits have the following problems:

• They do not properly encapsulate anything. Their private members are exposed to any class using the trait.
  We don't realize this as a problem in Drupal, because we got used to making everything protected instead of private. But this is not the way it should be, imo: #2727011: [policy, no patch] Private vs protected, and the role of inheritance
• They do not implement anything, and thus the IDE cannot tell us, when writing a trait, whether it matches with the respective interface.
  (In theory, the IDE (and PHP itself) can tell us for a class that uses a trait and implements an interface, that something is wrong. But this is not as comfortable as having this info directly on the trait. And PhpStorm currently fails at it.)
• They do not have a constructor. Or, they may have methods that are meant as constructors, but it is not trivial to make sure that initialization methods of all traits are called on object instantiation.
• Classes using a trait have access to and responsibility for private member variables that are in a separate file. They will have a hard time to make sure these variables are always in a valid state, and properly initialized. (yes, this somewhat overlaps with the previous point)

The preferred technology for reusing functionality should be composition, with interfaces to define contracts between classes.
Ideally, such classes and the interfaces should be small enough that reusing or overriding a part of a class is almost never necessary. Instead, one can write an alternative implementation from scratch.
Properties can be private, because such classes are not designed for inheritance.

There is still a case for inheritance: Sometimes the contract between the problem-specific part and the reusable part is so awkward or one-off or fragile that it does not deserve an interface. E.g. a public method in a base class could sanitize the parameter values, and then call a protected method with the sanitized arguments. No public method wants to rely on these sanitized argument values, but for a protected method that is only ever called from one place, this is ok.
Still, the implementation details (non-public members) of the base class would be private, to seal them off from the child class. Only one or few methods would be protected, as the API that the base class exposes to child classes.

Most classes in Drupal core are still far away from this.
With the average class size in Drupal, the desire to reuse parts of a class at random becomes inevitable, which is why so much is protected instead of private.
If we have already given up on encapsulation, by using protected all over the place, then traits cannot make it any worse. And in fact they can give some benefits.
But I would prefer this effort going into a more compositional architecture.

In the "ideal world" described further above, there is still a case for bigger interfaces with more methods. E.g. a SelectQuery really wants to expose a lot of methods for convenience, so the consuming component does not have to deal with microcomponents. Users of a public API don't want to call $query->getCondition()->where(..), but rather just $query->where(..).
Such classes can be implemented as thin compositional wrappers, which reduces the need for inheritance and traits. But I would not totally exclude traits to play a role for this case.

See my post here, if you are interested: http://programmers.stackexchange.com/questions/249583/why-not-apply-inte... ("Consumers: Unknown / external / 3rd party")

So yeah, why not use some traits where it makes sense. I just disagree with the original premise that we are basing this on.

Any updates about this? The last comment #85 was 4 years ago. I think traits are useful to tidy up \Drupal::foo() usages in tests at least.

I think the discussions here need to have a purpose. Cleaning up \Drupal or something like in tests only adds more for people to learn and to know. Traits are not the most discoverable thing.

At some point we need to get to the point where we can use Symfony's container as intended and use all their optimisations. The biggest part of that is moving away from public services and using the container as a service locator.

The traits being discussed here with setters for testability take us further away from that. And worse the setters have a side effect of making objects have a mutable state that they wouldn't have with constructor injection.