EntityType should put unknown properties into an array

I'm not too concerned about the public properties, but this does solve that.
I wonder if this will really be better in the end though.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -15,188 +15,57 @@
   +   *   - 'static_cache': (bool) Indicates whether entities should be statically
   

   These shouldn't have quotes around them.

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -15,188 +15,57 @@
   -   * Indicates whether entities should be statically cached.
   -   *
   -   * @var bool
   -   */
   -  protected $static_cache;
   

   I mainly worry about entity type authors, as this makes the annotation keys even harder to find.

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -326,7 +193,9 @@ public function getController($controller_type) {
   -    $this->controllers[$controller_type] = $value;
   +    $controllers = $this->getControllers();
   +    $controllers[$controller_type] = $value;
   +    $this->set('controllers', $controllers);
   

   This kinda sucks.

4. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -358,42 +229,43 @@ public function setList($class) {
   -    return $this->admin_permission ?: FALSE;
   +    return $this->get('admin_permission', FALSE);
   

   This is a wash, either way.

5. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -439,77 +313,77 @@ public function setLabelCallback($callback) {
   -    return !empty($this->translatable);
   

   I wonder if this will present a problem (isset vs empty)? Hopefully not.

So, this issue and #2169853: EntityType Annotation does not extend AnnotationBase are sort of similar and also sort of opposed.

One of the observations I've made here is that if the EntityType annotation moves to AnnotationBase, we should probably enumerate the properties common to all Entity Types on it. I'm doing some very simple filtering of the values passed to the annotation and putting any that are not also properties into an array together. With regard to how this actually affects the Annotation, I fell it's the best of all worlds since we can continue to add non-property elements to the annotation to extend the API in contrib, but we also set a standard for every required property.

The flip side to all of this is 2 fold:

1. With regard to the values that are passed into the EntityTypeInterface class upon instantiation (during AnnotationInterface::get()) I'm a little irritated that I have to refactor the Annotation into an array again. It feels like 1 step forward, 2 steps back.
2. I'm not sure I see the purpose of the EntityType annotation class AND the EntityType (interface) class that we use as a default for all entities. We can still override what is returned with an entity_type_class property, and if that's set instantiate and return it, but it seems to me the Annotation and Interface class could be one in the same.

This being the first discovery methodology to leverage class instances as definitions puts us in unexplored territory (at least within Drupal 8) and I want to keep an open mind about what the implementation could/should do. Still that being said, moving to a pure array definition of the "properties" feels like a drastic step backwards imo, and I'd prefer we not do that. I'm not really making an argument with regard to the specifics of the "entity system" itself, I'm speaking more in terms of just plugins in general.

Eclipse

First of all I'm a lil bit concerned that we OOP speaking think that it's ok to add properties that are not defined by the class, if you as user wanna add properties to a class that isn't defined, extend it and add your own properties and functions for it if needed. Otherwise it's will be hard to say what kind of object is this.

So in my opionon the current solution is not done correctly by letting any properties be added.

IF this is still something that is wanted it's clearly not good as it is since the properties not defined by class will be public but the solution to add a property that will hold any user added property in a associativ array.

IMHO not storing public stuff is still the right thing to do.

Especially because it also breaks some of the internal PHP optimizations: #2531564: Fix leaky and brittle container serialization solution

What public stuff? It's all protected. What is broken here?

Hope we didn't care about BaseFieldOverrideStorage too much...

I love checking DrupalCI for live results.
Fixing EntityTestUpdate for #2100343: Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route'.

<3

I guess I'll open a test-only issue to finish this... Next patch

The docs on content_translation_entity_type_alter() *tell* you to break this by adding arbitrary keys. :(

Here is an example of a fix for that (and an example for contrib). The obvious problem here is that only one module can set entity_type_class...

+++ b/core/modules/migrate/src/Entity/Migration.php
@@ -24,7 +24,7 @@
  *   label = @Translation("Migration"),
- *   module = "migrate",
+ *   provider = "migrate",
  *   handlers = {

Can be removed, set automatically?

Other than that, this approach doesn't make sense to me. The class approach doesn't work IMHO, that allows to set exactly one additional property. It's quite common for modules to extend this, we just have the luxury in core to actually be able to define those on the annotation class. Contrib doesn't. common_reference_target is entity_reference specific and I know that entity_reference_revisions adds a parallel property for that, field_ui_base_route is obviously field_ui specific, permission_granularity is also a (currently at least) a content translation specific thing,

It's also an API change.

Two alternatives:

* Transparently store everything that is not a defined property in $this->additional or some other property. No API change.
* Offer separate methods to set arbitrary flags. If combined with enforcing it like here, it would be an API change too.

Transparently store everything that is not a defined property in $this->additional or some other property. No API change.

That was my first suggestion to @damiankloip on IRC, I think that is the way forward.

So the only problem with those tests.... They pass with or without the test.
Is there any way to assert the PHP 5.4 property optimization thing?

Thanks!

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -233,6 +233,13 @@ class EntityType implements EntityTypeInterface {
+  protected $additional = [];

So this could potentially break any entity which defines additional already right? So there is some potential disruption.

@alexpott: No, not an entity. It would need to be a custom entity type annotation class, I'm not even sure that is supported with the simple annotation parser that we use now.

@catch asked in IRC

I don't think there are upgrade path implications, but do we actually rely on additional in core?
i.e. what breaks if we just don't add extra crap?

Can we just remove support for setting undeclared properties?

For #31

Ah of course ... hook_entity_type_alter() for example content_translation_entity_type_alter().

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 16f0ab8 and pushed to 8.0.x. Thanks!

I ponder upgrade path and other considerations I think this is safe to do.