Follow-up to #2432791: Skip Config::save schema validation of config data for trusted data

Problem/Motivation

#2432791-83: Skip Config::save schema validation of config data for trusted data has an improvement and has been missed as part of #2432791: Skip Config::save schema validation of config data for trusted data

Proposed resolution

Fix it.

Remaining tasks

  • Review
  • Commit

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes Issue category Bug because major performance/DX/UX impact on config import Issue priority Critical because of the massive performance improvements to the installer and cold cache view of node/1

-->

Files: 
CommentFileSizeAuthor
#26 interdiff.txt892 bytestim.plunkett
#26 2483407-config-export-26.patch28.57 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,818 pass(es), 26 fail(s), and 31 exception(s). View
#24 2483407-config-export-24.patch28.63 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#18 interdiff.txt2.52 KBtim.plunkett
#18 2483407-18.patch28.68 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,445 pass(es), 30 fail(s), and 30 exception(s). View
#12 interdiff.txt2.98 KBtim.plunkett
#12 2483407-12-FAIL.patch27.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#12 2483407-12-PASS.patch28.66 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#10 2483407-10.patch27.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#10 interdiff.txt21.56 KBtim.plunkett
#6 interdiff.txt5.63 KBtim.plunkett
#6 2483407-6.patch10.93 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#3 use_configuration-2483407-3.patch11.83 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,258 pass(es), 3 fail(s), and 0 exception(s). View

Comments

vijaycs85’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update
alexpott’s picture

Title: Improve ConfigEntityBase class » Use configuration schema in ConfigEntityType::getPropertiesToExport()

Re-titling so the issue is a bit more specific.

See #2432791-105: Skip Config::save schema validation of config data for trusted data - I'm not convinced about this change.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
11.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,258 pass(es), 3 fail(s), and 0 exception(s). View

Something like this.

tim.plunkett’s picture

Component: configuration system » configuration entity system

Right now in HEAD, if you call toArray() twice in a row, it checks the schema twice. Not true with this patch.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
@@ -165,9 +166,9 @@ protected function checkStorageClass($class) {
-  public function getPropertiesToExport() {
...
+  public function getPropertiesToExport($entity_id) {

Still not convinced about the coupling of ID to the entity type method.

re #4 on the second check it will be cached in schema so it's not a big saving - right? Also it only checks schema if you don't use the annotation.

tim.plunkett’s picture

FileSize
10.93 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
5.63 KB

Duh, we don't need that because config schema is smarter than us.

Because there's no way anyone is sneaky enough to define a per-entity-id schema, right? (please, right!?)

The last submitted patch, 3: use_configuration-2483407-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2483407-6.patch, failed testing.

yched’s picture

Heh, I so much wish accessing the schema didn't require a valid entity ID, alas currently it does.
Because of the schema is at prefix.*.*.* (with an unknown number of *s)

See #1977206: Default serialization of ConfigEntities, where I hit that wall as well (comments #26 to #31).
In short, we know the config prefix for an entity type, but not the number of dynamic parts in its IDs, which is what config schemas are keyed by...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
21.56 KB
27.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Yes, @alexpott and I had a conversation about that. He also mentioned you had an idea about tying the schema to the entity type more closely, which is what I've done here.

alexpott’s picture

I think I like this change - it'd be good to get @yched's thoughts on this because I think this issue has caused him head scratches in the past.

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -159,6 +159,20 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $is
+  public function getDefinitionForEntityType($entity_type_id) {
+    $id = 'undefined';
+    foreach ($this->getDefinitions() as $definition_id => $definition) {
+      if (isset($definition['config_entity_type']) && $definition['config_entity_type'] === $entity_type_id) {
+        $id = $definition_id;
+        break;
+      }
+    }
+    return $this->getDefinition($id);
+  }

Given the number of definitions it might be worth building up a static map here.

tim.plunkett’s picture

FileSize
28.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
27.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
2.98 KB

Agreed.

yched’s picture

Huge +1 on having a more direct relation between an entity type and its config schema :-D

I would have thought that the simpler way to have that relationship is to put the missing info on the entity type (since it already has the "config_prefix"), rather than pointing back from the schema to the config entity type ID (which then could be inconsistent with what the config entity's "config_prefix" says) ?

It looks like the proposed change means a hard BC break anyway (your config entity stops working if its config schema doesn't have a 'config_entity_type' entry). If we're ready to break APIs for this (which I'm all for), then replacing
'config_prefix' = 'my.prefix'
by
'config_pattern' = 'my.prefix.*.*.*'
in the entity annotation would be more intuitive, faster, and with no risk of inconsistencies ?

The "config_pattern" directly points to the correct schema entry, and you can deduce the config_prefix from it (the part before the first '.*')

The last submitted patch, 6: 2483407-6.patch, failed testing.

tim.plunkett’s picture

The patch as it stands is only a BC break for those NOT using the new config_export annotation. If that exists, this code never runs.
(Of course, that's brand new and I doubt anyone is using it yet)

Changing config_prefix to config_pattern would be a BC break for everyone.

Honestly I am +1 to being able to look at a config schema and see the entity type it's for. If your entity type has some weird prefix, it can be hard to track backwards from the schema to the exact entity type...

yched’s picture

The patch as it stands is only a BC break for those NOT using the new config_export annotation. If that exists, this code never runs.

Well, the new public function getDefinitionForEntityType() breaks if the config schema for your entity type doesn't point back to the entity_type_id. OK, the method is new, but still, the definition of "being a valid config entity for which public methods work" does change, that seems like a BC break to me ?

If your entity type has some weird prefix, it can be hard to track backwards from the schema to the exact entity type...

Yeah, dunno. This is still putitng two separate related informations on both ends of the relationship (config_prefix is on the Entity, entity_type is on the schema, which also implictly includes a config_prefix, which might or might not be the same). Feels like a convoluted denormalized mental model, esp. when one single information in one place would be enough :-)

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -159,8 +168,26 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $is
+      $this->entityTypeDefinitions[$entity_type_id] = $this->getDefinition($id);

let's just store the definition ID instead so we don't have unnecessary duplication between $this->definitions and $this->entityTypeDefinitions.

tim.plunkett’s picture

FileSize
28.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,445 pass(es), 30 fail(s), and 30 exception(s). View
2.52 KB

#16, I'll take a look at rewriting things in the other direction tomorrow.

#17, sure, okay

The last submitted patch, 12: 2483407-12-PASS.patch, failed testing.

The last submitted patch, 12: 2483407-12-FAIL.patch, failed testing.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -169,7 +169,7 @@ public function getDefinition($base_plugin_id, $exception_on_invalid = TRUE, $is
+    if (TRUE || !isset($this->entityTypeDefinitionIds[$entity_type_id])) {

debug?

Status: Needs review » Needs work

The last submitted patch, 18: 2483407-18.patch, failed testing.

The last submitted patch, 10: 2483407-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
28.63 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 24: 2483407-config-export-24.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
28.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,818 pass(es), 26 fail(s), and 31 exception(s). View
892 bytes

Status: Needs review » Needs work

The last submitted patch, 26: 2483407-config-export-26.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.