Problem/Motivation

Configuration entity properties should be protected. At the moment we determine which entities to export using a mix of reflection and hardcoding in the toArray() method. We can use schema to do this instead.

We have fixed the public access of ID many times, but it keeps creeping back in.

Proposed resolution

  • Use configuration schema to determine which properties to export
  • Remove unnecessary toArray implementations
  • Change a config entity to have a protected ID property to prevent regressions

This solution has the beneficial side effect of ensuring the order or the root keys in the exported configuration matches its configuration schema.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#51 2256679.51.patch50.53 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,505 pass(es). View
#51 42-51-interdiff.txt2.18 KBalexpott
#42 2256679.42.patch47.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,286 pass(es). View
#42 40-42-interdiff.txt2.46 KBalexpott
#40 2256679.40.patch45.47 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#37 2256679.37.patch45.46 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256679.37.patch. Unable to apply patch. See the log in the details link for more information. View
#33 2256679.33.patch45.47 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256679.33.patch. Unable to apply patch. See the log in the details link for more information. View
#33 29-33-interdiff.txt17.75 KBalexpott
#29 2256679.29.patch30.18 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,875 pass(es), 1 fail(s), and 3 exception(s). View
#29 28-29-interdiff.txt593 bytesalexpott
#28 2256679.28.patch29.97 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#24 2256679.24.patch31.52 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,823 pass(es). View
#24 20-24-interdiff.txt4.37 KBalexpott

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
614 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,328 pass(es), 40 fail(s), and 0 exception(s). View
1.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,311 pass(es), 162 fail(s), and 92 exception(s). View

The last submitted patch, 1: entity-2256679-1-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 1: entity-2256679-1-PASS.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

1: entity-2256679-1-PASS.patch queued for re-testing.

tim.plunkett’s picture

FileSize
4.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,320 pass(es), 11 fail(s), and 0 exception(s). View

The last submitted patch, 1: entity-2256679-1-PASS.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: entity-2256679-4.patch, failed testing.

Berdir’s picture

+++ 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...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,430 pass(es). View

Mehhhhh.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Works for me :)

alexpott’s picture

Title: Stop assuming entity properties are public » Stop assuming config entity properties are public
Status: Reviewed & tested by the community » Needs work
+++ 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.

Berdir’s picture

Assigned: Unassigned » alexpott
Status: Needs work » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.86 KB
14.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,619 pass(es), 2 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 13: 2256679.13.patch, failed testing.

alexpott’s picture

FileSize
2.57 KB
17.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,679 pass(es). View

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

alexpott’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

I think the recent changes are really cool, but are very much out of scope here...
If you want to come up with a better name for the issue and expand the scope, I guess there's nothing I can do about it.

alexpott’s picture

FileSize
11.99 KB
31.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,671 pass(es). View

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

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

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -240,20 +241,58 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
+    catch (SchemaIncompleteException $e) {
...
+    if (!isset($definition['mapping'])) {
+      throw new SchemaIncompleteException(String::format('Missing root mapping definition in config schema for !name', array('!name' => $config_name)));
+    }

What is the benefit of moving this out to another method, and using exceptions to communicate between the two?

Also, do you think we can add an @todo here to remove this? Ideally we'd never have public properties or properties without a schema.

alexpott’s picture

FileSize
2.88 KB
30.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,740 pass(es). View
2.88 KB
30.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Good point. Cleaned up the code too.

alexpott’s picture

For some reason duplicate uploads :)

alexpott’s picture

Issue summary: View changes

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.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -240,20 +240,45 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    +      // @todo Remove once migration config entities have schema
    +      //   https://drupal.org/node/2183957.
    +      $class_info = new \ReflectionClass($this);
    +      $keys = array();
    +      foreach ($class_info->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {
    +        $keys[] = $property->getName();
    +      }
    

    Is it really just migration? There are two classes there, but migrate_drupal's entity extends migrate's entity, is it worth moving this hack to that class directly and making the API break now?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -240,20 +240,45 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    +    foreach ($keys as $name) {
    +      if ($name == $id_key) {
    +        $properties[$name] = $this->id();
    +      }
    

    I think we should document why we're calling id() here (for entities with compound IDs like FieldConfig).

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -240,20 +240,45 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    -    // Add protected dependencies property.
    -    $properties['dependencies'] = $this->dependencies;
    

    Right now this is forcibly added to all config entities, which isn't clean, but is reliable.

    Now it relies on the schema to define dependencies.

    This seems like a problem:

    $ grep -r "  dependencies:" * | grep schema | wc -l
          16
    $ grep -r " @ConfigEntityType" * | wc -l
          33
    
  4. +++ b/core/modules/field/lib/Drupal/field/Entity/FieldConfig.php
    @@ -725,7 +697,10 @@ public function entityCount($as_bool = FALSE) {
    -    return array_keys(array_intersect_key($this->toArray(), get_object_vars($this)));
    +    $properties = array_keys(array_intersect_key($this->toArray(), get_object_vars($this)));
    +    // Serialize $entityTypeId property so that toArray() works when waking up.
    +    $properties[] = 'entityTypeId';
    +    return $properties;
    

    This isn't immediately clear to me why we need this now. The comment is fine, but can you shed some light on why we need this change?

Also, we still need a better issue title.

alexpott’s picture

Title: Stop assuming config entity properties are public » Use config schema to determine which config entity properties to export regardless of whether they are public or protected
FileSize
4.37 KB
31.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,823 pass(es). View
  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()
tim.plunkett’s picture

I think we should fix up ConfigEntityBaseUnitTest::testToArray().
Not sure how that passed, honestly.

Gábor Hojtsy’s picture

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.

alexpott’s picture

Assigned: alexpott » Unassigned

@tim.plunkett not sure what you by

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

alexpott’s picture

FileSize
29.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Rerolled for PRS4

alexpott’s picture

FileSize
593 bytes
30.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,875 pass(es), 1 fail(s), and 3 exception(s). View

Forgot to commit a little bit of the reroll.

The last submitted patch, 28: 2256679.28.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 29: 2256679.29.patch, failed testing.

tim.plunkett’s picture

No idea what I meant in #25 either, sorry. If this gets back to green, I think its good to go.

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.75 KB
45.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256679.33.patch. Unable to apply patch. See the log in the details link for more information. View

#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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

While that somewhat diminishes the usefulness of the config_entity schema, it's still worth keeping around for the 4 other properties from ConfigEntityBase.

And this issue as a whole is still a big improvement.
+1

Gábor Hojtsy’s picture

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: 2256679.33.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
45.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2256679.37.patch. Unable to apply patch. See the log in the details link for more information. View
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

vijaycs85’s picture

Issue tags: +Configuration schema

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 2256679.37.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
45.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 2256679.40.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.46 KB
47.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,286 pass(es). View

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

Gábor Hojtsy’s picture

+++ 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.

vijaycs85’s picture

. 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.

alexpott’s picture

@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.

Gábor Hojtsy’s picture

@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.

Jose Reyero’s picture

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...

alexpott’s picture

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.

Jose Reyero’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Rerolling for the CommentType config entity.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.18 KB
50.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,505 pass(es). View

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.

Berdir’s picture

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.

Jose Reyero’s picture

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?

  • Commit 0c33183 on 8.x by catch:
    Issue #2256679 by alexpott, tim.plunkett: Fixed Use config schema to...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Berdir’s picture

@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.

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

Category: Bug report » Task
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Missing change record

Anyone calling parent::toArray without a schema is in for a rude surprise (SchemaIncompleteException). This needs a change notice.

Gábor Hojtsy’s picture

Category: Task » Bug report
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Missing change record

Status: Fixed » Closed (fixed)

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

yched’s picture

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 ?

Gábor Hojtsy’s picture

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.