Use config schema to determine which config entity properties to export regardless of whether they are public or protected

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Entity/ConfigTest.php
@@ -91,6 +91,7 @@ class ConfigTest extends ConfigEntityBase implements ConfigTestInterface {
     $properties = parent::toArray();
     $protected_names = array(
+      'id',
       'protected_property',
     );

This will affect the order of properties and move the id (almost) at the bottom, which is a bit weird. And it's what's causing all those import and diff fails, because default config needs to be updated to look the same as one written through the API...

Works for me :)

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Entity/ConfigTest.php
@@ -89,7 +89,10 @@ class ConfigTest extends ConfigEntityBase implements ConfigTestInterface {
-    $properties = parent::toArray();
+    $properties = array(
+      'id' => $this->id(),
+    );
+    $properties += parent::toArray();

@dawehner and I wondered about why this wasn't being done on ConfigEntityBase and then @dawehner wondered if we couldn't use schema to do all this for us?

I'm having a look at doing this.

This is how we do it right now, just look at how many classes subclass ConfigEntityBase::toArray() to do this?

Happy to look at using the schema/typed data for doing this, but there is no reason to block this issue on that?

Back to RTBC and assigning to @alexpott for feedback.

To me this is totally related because have to do:

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Entity/ConfigTest.php
@@ -89,7 +89,10 @@ class ConfigTest extends ConfigEntityBase implements ConfigTestInterface {
   public function toArray() {
-    $properties = parent::toArray();
+    $properties = array(
+      'id' => $this->id(),
+    );
+    $properties += parent::toArray();
     $protected_names = array(
       'protected_property',
     );

to protect the ID looks really odd.

Here's a patch exploring the idea of using config schema - I think it works out really nicely because now the order of properties is controlled by the schema. I've removed the date config entities toArray() method to show what is possible.

Fixing tests - FilterFormats have an interesting property 'roles' that is only used during config install!

Well the default toArray is assuming that config entity properties are public :)

Patch posted just to show how much code we can remove.

Good point. Cleaned up the code too.

For some reason duplicate uploads :)

Updated issue summary to reflect the changes - I think that the changes to toArray() in the original patches on this issue make the further changes to use schema in toArray() in scope. The current solution is preferable rather that more and more custom implementations of toArray() when we already have this information to hand.

1. Nice idea
2. Fixed
3. Actually not every config entity has a need for dependencies key for example custom block types, date formats
4. We need this since entityTypeId is used in getEntityType() which is now called in toArray() - entityTypeId is set in the __construct() but toArray() is called before __construct() in __wakeup()

I like the concept of the patch overall. It makes it even more useful to define schema for config entities otherwise you need to define the toArray() method too. On top of the already existing type-casting benefit.

@tim.plunkett not sure what you by

I think we should fix up ConfigEntityBaseUnitTest::testToArray().

Rerolled for PRS4

Forgot to commit a little bit of the reroll.

#2273631: Unify config entity schemas with a base schema type added the config_entity schema data type. Unfortunately this included id<code> and <code>label - these are not present on all config entities. Since we're now using schema in export this exposes this mistake. Schema's should not contain definitions for things that can never exist. Also block config entities do not have a label property.

Yeah well, the practical application of the schema is so that any extra keys defined would not be an issue, except of course this application of the schema. If you look at eg. the mapping handler, it would look at the *data* and apply the schema to it, so it would just ignore extra schema elements defined. I think we should somehow figure out if we want to support optional elements or not because we are getting to be inconsistent. I think this issue is fine to get in, but we need to straighten optionality somehow.

First, rewinding head to replay your work on top of it...
Applying: Using schema whilst saving config entities
Applying: Fix tests
Applying: Remove all the unnecessary ::toArray()
Applying: Remove unnecessary ::toArray implementations
Applying: Refactor schema usage in config entity base
Applying: Refactor schema usage in config entity base
Applying: @timplunkett's review
Applying: Fix schema
Applying: Fix config_entity schema
Using index info to reconstruct a base tree...
A	core/modules/block/custom_block/config/schema/custom_block.schema.yml
M	core/modules/contact/config/schema/contact.schema.yml
M	core/modules/field/config/schema/field.schema.yml
M	core/modules/system/config/schema/system.schema.yml
M	core/modules/user/config/schema/user.schema.yml
Falling back to patching base and 3-way merge...
Auto-merging core/modules/user/config/schema/user.schema.yml
Auto-merging core/modules/system/config/schema/system.schema.yml
Auto-merging core/modules/field/config/schema/field.schema.yml
Auto-merging core/modules/contact/config/schema/contact.schema.yml
Auto-merging core/modules/block_content/config/schema/block_content.schema.yml

Straight reroll... back to rtbc

First, rewinding head to replay your work on top of it...
Applying: Using schema whilst saving config entities
Applying: Fix tests
Applying: Remove all the unnecessary ::toArray()
Using index info to reconstruct a base tree...
M	core/modules/field/src/Entity/FieldInstanceConfig.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/field/src/Entity/FieldInstanceConfig.php
Applying: Remove unnecessary ::toArray implementations
Applying: Refactor schema usage in config entity base
Applying: Refactor schema usage in config entity base
Applying: @timplunkett's review
Applying: Fix schema
Applying: Fix config_entity schema

Simple git reroll... back to rtbc.

Fixed the new FieldInstanceConfigEntityUnitTest::testToArray() - which is why #40 failed. Setting back to rtbc since all changes were to the unit test.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -240,20 +241,36 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
+        $properties[$name] = $this->get($name);

So this is what makes the schema required to exactly match the list of properties and extra elements defined are not ignored. Why not check if there was such a property and just ignore the schema definition if there is none?

That would let us keep using the schema as not necessarily a 100% concrete match for the data. I brought up this issue with reyero and vijaycs85 in the multilingual meeting yesterday and they were concerned that this changes the expectation about the schema from a superset of what may be in the data to an exact match of what may be in the data.

. I brought up this issue with reyero and vijaycs85 in the multilingual meeting yesterday and they were concerned that this changes the expectation about the schema from a superset of what may be in the data to an exact match of what may be in the data.

My main concern around this is, we created schema with all possible config items in config entities (like views plugin) and we tried to cover all possible elements as data type. So even some of the elements not used, it will avoid duplication by placing in data type.

If we are moving them back to individual schema file, then we may need to make sure we don't break any, as core tests config entities may not cover all cases.

@vijaycs85 - all config entities are used by tests so if we were missing any of the properties we'd know pretty quickly - especially since we're using the schema to decide what properties to export. We have to be able to get a list of things to export - either from code, reflection, configuration schema or annotations. We have this information in schema.

That would let us keep using the schema as not necessarily a 100% concrete match for the data.

This is the bit that really concerns me too - we need to be able to mark schema keys as optional otherwise we're not going to be able to use schema in all situations where they might be valuable.

@alexpott: well, assumptions in the typed config manager right now is that if something is defined in the schema but not in the data, then its ignored, see Mapping's parse() method. That assumption is a basic defining factor as to how people need to understand how what they provide in schema would behave. Your code here makes very different assumptions. I'm not saying either assumption is correct just that they contradict. I'm totally fine with either definition but with this patch we would have two different understandings of how a definition of schema applies.

Hi, having similar concerns a @Gábor and @vijaycs85, I want to add my 2 cents:

Do we really need to export/import NULL properties?

Can't we fix this by adding a schema 'required' property and if not required, just exporting the property if it is set? (that is already in TypedData definitions btw, so it is just adding it into the schema, not into D8)

- About @Gábor #46, right, but we need to add that having it in the data but not in the schema then it is ignored too (for Mappings), that may be a concern if any other module adds some properties to our entity object. (I'd like to fix that though).

Also thinking that by exporting / importing all properties without checking with isset(), we may end up with a different object to what we started from, with properties that were not set but end up set to null or 0 or '' (saved configuration is typed)

Overall I like this patch's idea a lot, just concerned about these details, and also about the restriction it imposes on schema definitions by working with different premises as generic config objects (not entities), that have a lot of optional properties...

Well we need to generate a list of properties to export - we can choose from:

• custom toArray() method for each config entity
• a new annotation key in for config entities
• a new property annotation akin to Doctrine - each exportable property would be annotated
• use configuration schema

For me the last choices are the best and actually given that the inheritance of config_entity schema type is messing with the ordering of the keys anyway perhaps a property annotation is the best way to go because then we control ordering very simply - through the order of the properties on the class.

Making properties protected on config entities is important because without them we have no api and no way to change things once D8 is released. If everyone is accessing properties through a method we have a chance to produce backwardly compatible code and change things under the bonnet.

Agree with making properties protected being a priority, let's commit this first and discuss about further schema options later.

The only change this introduces from the config schema perspective is: 'schema for config entities must exactly match the data' which is manageable for any future validation, etc.. scenario.

Rerolling for the CommentType config entity.

Putting back to rtbc since the CommentType changes are a conversion to the new method. They also show the importance of making everything protected asap.

Making properties protected on config entities is important because without them we have no api and no way to change things once D8 is released. If everyone is accessing properties through a method we have a chance to produce backwardly compatible code and change things under the bonnet.

Honestly, that's not really true because it's a config entity and the properties of a config entity is part of the config schema which we can't change I thought?

Anyway, I agree that the schema should not contain additional elements, because there are for example ideas to be able to use config entities as typed data by relying on the schema, that would not be reliably possible if there are things in there that do not actually exist.

Hey @Berdir,

there are for example ideas to be able to use config entities as typed data

That sounds interesting, could you point me to any issue, please?

This looks great and cleans things up a lot. Committed/pushed to 8.x, thanks!

@Jose: The idea is that with #2002138: Use an adapter for supporting typed data on ContentEntities, the same could be done for config entities, by reading the typed data definition in from the schema and then implementing it on top of it. There's no issue yet for it I think.

Added this to the schema docs at https://drupal.org/node/1905070 as another use case:

Using the knowledge embedded in the configuration schemas about what is stored of a configuration entity, the default persistence implementation for configuration entities requires a configuration schema for the configuration entity, so the right properties are exported with the types defined. Although it is better to provide configuration schemas, if you really don't want, implement the toArray() method in your configuration entity implementation to not require schema for saving configuration entities of your type

See diff at https://drupal.org/node/1905070/revisions/view/7276615/7351805

Added https://drupal.org/node/2288297.

Feedback welcome in #1977206: Default serialization of ConfigEntities - see #26 over there (and #27 for why that patch would be worthwhile)

In short : ConfigEntityBase::toArray() now doesn't work for freshly created entities that don't have an id() yet.
entity_create('entity_type', $values)->toArray() fails with a "config schema not found" exception.

The config schema system has no way to know the name of the schema definition entry for a given entity type ('filter.format.*', or 'field.instance.*.*.*', with the right number of '*' parts). It only guesses by trial and error from an actual config name like field.instance.foo.bar.baz, but there is no way to get the schema for an entity that can't provide a valid config name yet.

Arguably that's a bug ? EntityInterface::toArray() states "Returns an array of all property values". Nothing in there says the entity needs to be in this or that state ?

Based on discussion on last week's CMI meeting, I opened #2361539: Config export key order is not predictable for sequences, add orderby property to config schema to take this to the next level.