Proposed commit message

Issue #2432791 by alexpott, vijaycs85, tim.plunkett, joshtaylor, Fabianx, Berdir, yched, bojanz: Skip Config::save schema validation of config data for trusted data

Problem/Motivation

Configuration Import is slow and needs a lot of memory.

A lot of that is the schema validation in Config::save. We also recalculate the dependencies for all configuration entities.

Proposed resolution

Add a trusted flag to Config::save() / ConfigEntityBase::save() so that casting to schema and calculating dependencies can be skipped for module and theme installation.

This does not affect schema coverage during testing because we still have the config save event listener that checks all configuration - and if any module ships with invalid config this will find it :)

Furthermore, once we implement import validators for dependencies and schema we can skip them during import making that quicker too. See

Remaining tasks

  • Review
  • Commit

User interface changes

None

API changes

  • Add a trust_data flag to Config::save()
  • Add ConfigEntityInterface::trustData() and ConfigEntityInterface::hasTrustedData()
  • Add config_export annotation to configuration entity types
  • Add ConfigEntityTypeInterface::getPropertiesToExport()

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
#97 2432791.97.patch70.88 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,169 pass(es). View
#97 94-97-interdiff.txt2.66 KBalexpott
#94 2432791.94.patch68.55 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,173 pass(es), 3 fail(s), and 0 exception(s). View
#94 85-94-interdiff.txt3.76 KBalexpott
#85 interdiff.txt948 bytestim.plunkett
#85 2432791-85.patch65.55 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,935 pass(es). View
#83 interdiff.txt5.76 KBtim.plunkett
#83 2432791-83.patch65.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,931 pass(es), 1 fail(s), and 0 exception(s). View
#81 2432791.81.patch62.84 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,928 pass(es). View
#71 2432791.70.patch64.73 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,906 pass(es). View
#71 67-70-interdiff.txt1.68 KBalexpott
#67 2432791.67.patch63.75 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,916 pass(es). View
#65 2432791.65.patch63.66 KBjoshtaylor
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,915 pass(es), 1 fail(s), and 0 exception(s). View
#60 2432791.60.patch63.74 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,397 pass(es). View
#60 57-60-interdiff.txt9.15 KBalexpott
#57 2432791.57.patch59.9 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
#57 56-57-interdiff.txt648 bytesalexpott
#56 53-56-interdiff.txt21.31 KBalexpott
#53 2432791.53.patch40.16 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,339 pass(es), 1 fail(s), and 81 exception(s). View
#53 51-53-interdiff.txt11.26 KBalexpott
#51 2432791.51.patch28.9 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,343 pass(es). View
#51 49-51-interdiff.txt3.79 KBalexpott
#49 2432791.49.patch25.11 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,351 pass(es). View
#49 2432791.47-49-interdiff.txt2.39 KBalexpott
#47 2432791.47.patch24.88 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,310 pass(es). View
#47 2432791.45-47-interdiff.txt1.1 KBalexpott
#45 33-45-interdiff.txt2.78 KBalexpott
#45 2432791.45.patch23.78 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,347 pass(es), 73 fail(s), and 52 exception(s). View

Comments

Fabianx’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
5.74 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

Something like this?

Note that this patch should fail because we have tests that schema are enforced during module install.

alexpott’s picture

Status: Needs work » Needs review
FileSize
719 bytes
5.75 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

Whoops.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
8.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,757 pass(es), 87 fail(s), and 4 exception(s). View

Will still fail (ithink) but less :)

catch’s picture

Issue tags: +Performance
alexpott’s picture

Issue tags: +Configuration system
alexpott’s picture

Issue summary: View changes
alexpott’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.1 KB
16.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,301 pass(es), 2 fail(s), and 0 exception(s). View

Fingers-crossed.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -559,4 +561,28 @@ public function isInstallable() {
+   * {@inheritdoc}
+   */
+  public function save() {
+    $return = parent::save();
+    $this->trustedData = FALSE;
+    return $return;
+  }

Why do we consider something as not trusted, once we have saved it? Maybe add some line of documentation to explain that behaviour.

Gábor Hojtsy’s picture

The IS says "Add a trusted flag so that these steps can be skipped for module installation.". Would be good to extend the IS on what is the scope of the proposed trusted flag, what would it mean, etc. Likewise, as @dawehner pointed out the code would need some docs too. Its hard to evaluate this patch without knowing its scope.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.78 KB
21.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,299 pass(es), 1 fail(s), and 0 exception(s). View

Not sure why this change is exposing the bug in the contact module but it is - it is probably due to removing the calculate dependencies change.

Fabianx’s picture

Do you think views could use this check too to skip the $this->addCacheMetadata() step or should this be follow-up in another issue?

I like the patch a lot already!

alexpott’s picture

Status: Needs work » Needs review
FileSize
22.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,295 pass(es). View
441 bytes

Fixed the last test fail.

vijaycs85’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -559,4 +561,28 @@ public function isInstallable() {
    +  public function setTrustedData() {
    

    Minor: makeTrusted/markTrusted would be more appropriate.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -203,4 +203,25 @@ public function getDependencies();
    +  public function getTrustedData();
    

    Minor: isTrusted?

  3. +++ b/core/lib/Drupal/Core/Config/ImmutableConfig.php
    @@ -44,7 +44,7 @@ public function clear($key) {
    +  public function save($trusted_data = FALSE) {
    

    Minor: $is_trusted = FALSE

  4. +++ b/core/modules/config/tests/config_test/config/install/config_test.types.yml
    @@ -2,7 +2,7 @@ array: []
    +float_as_integer: !!float 1
    

    !!float ?

Fabianx’s picture

#21: +1 to isTrusted() and markTrusted().

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Config.php
    @@ -203,22 +203,24 @@ public function clear($key) {
    +  public function save($trusted_data = FALSE) {
    

    s/$trusted_data/$is_trusted/

    It's a boolean, not data, the parameter name should match that.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -203,4 +203,25 @@ public function getDependencies();
    +   * @param bool $trust
    ...
    +  public function setTrustedData();
    

    There's no parameter?

bojanz’s picture

alexpott’s picture

FileSize
6.88 KB
21.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2432791.25.patch. Unable to apply patch. See the log in the details link for more information. View

Thanks for the reviews everyone.

re @vijaycs85 #21

  1. Changed to trustData()
  2. Changed to hadTrustedData()
  3. Changed to $has_trusted_data
  4. This ensure that Symfony's YAML parser returns a float and not an integer - this is part of the yaml spec - see https://github.com/symfony/symfony/pull/11976

re @Wim Leers #23

  1. Changed to $has_trusted_data
  2. Changed to trustData()
Berdir’s picture

Looks like drush si standard is about 45s vs. 43s with this, only did a single run, so might vary a bit.

I guess I was hoping for a bit more, but we still have runtime configuration changes, that need to be checked, and thefore we end up parsing/loading the config schema after every installed module anyway.

One obvious example is saving the module list in ModuleInstaller ;)

Actually, even if we e.g. do call it there with save(TRUE), we still need need the config schema for default configuration, because config entities need to config schema in toArray().

To summarize, while this makes things a bit faster, there is no way to avoid re-parsing config schema after every installed module.

Fabianx’s picture

It saves quite some memory with non-drush install though ...

Berdir’s picture

Sure, it helps, just not quite as much as I'd hoped it would :)

Berdir queued 25: 2432791.25.patch for re-testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
21.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,272 pass(es), 298 fail(s), and 249 exception(s). View

Rerolled.

alexpott’s picture

Status: Needs work » Needs review
FileSize
583 bytes
22.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,854 pass(es). View

This patch continues to find interesting things :)

vijaycs85’s picture

+1 to RTBC.

Berdir queued 33: 2432791.33.patch for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Works nicely here.

RTBC

vijaycs85’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

updating beta evaluation...

vijaycs85’s picture

Issue summary: View changes
joshtaylor’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
42.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,115 pass(es), 28 fail(s), and 30 exception(s). View
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
44.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,142 pass(es), 28 fail(s), and 29 exception(s). View
1.69 KB
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
44.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,141 pass(es), 0 fail(s), and 1 exception(s). View
515 bytes

oops... Missed one...

alexpott’s picture

Status: Needs work » Needs review
FileSize
23.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,347 pass(es), 73 fail(s), and 52 exception(s). View
2.78 KB

The rerolls from #39 and included a lot of unrelated change. Interdiff is to #33.

alexpott’s picture

FileSize
1.1 KB
24.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,310 pass(es). View

Fixing new test failures.

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
    @@ -304,11 +304,11 @@ protected function createConfiguration($collection, array $config_to_create) {
    -          $entity->save();
    +          $entity->trustData()->save();
    ...
    -        $new_config->save();
    +        $new_config->save(TRUE);
    

    I don't want to hold this up as the patch is really really good, but I still wonder why we use two different schemas to do the same thing ...

    That feels weird in a way.

    I forgot:

    Is there a reason for that?

  2. +++ b/core/lib/Drupal/Core/Config/ImmutableConfig.php
    @@ -44,7 +44,7 @@ public function clear($key) {
    -  public function save() {
    +  public function save($trusted_data = FALSE) {
    

    I think the parameter needs to be $has_trusted_data here as well.

alexpott’s picture

FileSize
2.39 KB
25.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,351 pass(es). View
  1. Because we can't change the signature of Entity::save() for this and we need to affect the entity storage so we have to pass this along.
  2. Fixed
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC then, thanks for the information!

Edit: Does this need the API change tag?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.79 KB
28.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,343 pass(es). View

We can stop the schema cache rebuilding during extension install too.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me!

I wonder if its almost easier to make trusted the default and opt-in for validation, but that can be a follow-up discussion.

I also wondered if you would like some new profiling for #51?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.26 KB
40.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,339 pass(es), 1 fail(s), and 81 exception(s). View

Done some per testing... I'm not sure this is worth it. I've fixed all the untrusted writes during both standard and minimal install. As @Berdir pointed out in #26 we need to do something about ConfigEntityBase::toArray().

Minimal install

Without the patch

Starting Drupal installation. This takes a while. Consider using the --notify global option. [1.24 sec, 6.69 MB]                                                  [ok]
Calling install_drupal(Object, Array) [1.24 sec, 6.69 MB]                                                                                                             [debug]
Installation complete.  User name: admin  User password: admin [7 sec, 49.89 MB]

With the patch

Starting Drupal installation. This takes a while. Consider using the --notify global option. [3.31 sec, 6.69 MB]                                                  [ok]
Calling install_drupal(Object, Array) [3.31 sec, 6.69 MB]                                                                                                             [debug]
Congratulations, you installed Drupal! [6.75 sec, 45.93 MB]

With all configuration writes are trusted.

Standard

Without patch

Calling install_drupal(Object, Array) [2.56 sec, 6.69 MB]                                                                                                             [debug]
Installation complete.  User name: admin  User password: admin [27.64 sec, 78.43 MB]

With patch

Starting Drupal installation. This takes a while. Consider using the --notify global option. [1.77 sec, 6.69 MB]                                                  [ok]
Calling install_drupal(Object, Array) [1.77 sec, 6.69 MB]                                                                                                             [debug]
Installation complete.  User name: admin  User password: admin [20.66 sec, 77.24 MB]

With all configuration writes are trusted.

Fabianx’s picture

Pre-liminary numbers on #53:

With patch

* 1 m - 1m 3 secs 'real time' without patch
* 23.* secs user time - without patch

With patch

* 48 - 51 secs 'real time'
* 16.* secs 'user time'

alexpott’s picture

Status: Needs work » Needs review
FileSize
21.31 KB

So the patch attached no longer uses schema to work out what properties to export to config from config entities. It would also allow camel case properties to be mapped to snake case keys in config.

@Fabianx can you profile this - I'm seeing a 10 second improvement during a standard install in Drush - but not a lot of memory improvements.

alexpott’s picture

FileSize
648 bytes
59.9 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

I left some debug in :(

bojanz’s picture

Nice! Note that I started a similar mapping in #2411967: Allow config entities to specify the config key <-> entity property mapping in order to resolve #1832630: [policy, then patch] Decide on coding standard for configuration keys of ConfigEntities, I'm fine with most of that work happening here instead.

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.15 KB
63.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,397 pass(es). View

Fixed some of the tests.

Fabianx’s picture

Issue tags: +D8 Accelerate Dev Days

Tagging

Fabianx’s picture

Issue tags: +Needs profiling

Needs some profiling, RTBC from my side unless something is missing ...

tim.plunkett’s picture

Adding this tag not because it improves DX, but because it regresses.

bojanz’s picture

Reviewed the patch.

The good things:
1) Improved performance, and the ability to mark data as "trusted".
This is major use case in Commerce 2.x because we import known valid data from our libraries (currencies, address formats, tax types, tax rates).

2) The ability to map entity properties to config keys. This resolves #1832630: [policy, then patch] Decide on coding standard for configuration keys of ConfigEntities, an issue we've been stuck on for years, which is a big source of core inconsistency, causing core to not respect its own coding standards, and core entity types to somethimes use camelCase properties, and sometimes not.
Most config entities have at least one property that will need to be mapped this way.

3) Much less important than #1 and #2, a config entity can now be saved before it has a schema declared, making it easier to get started with some prototype code. I'm not sure how important this is, but I'll "allow" the argument.

The bad things:
1) Config entity types now need to define another annotation key, listing their properties. This is another step, and duplication, since they will end up declaring the properties in schema as well. So, not exactly elegant.

However, I'm okay with it, since #2 basically mandates it (and the patch in #2411967: Allow config entities to specify the config key <-> entity property mapping started doing the same), and because performance.

joshtaylor’s picture

FileSize
63.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,915 pass(es), 1 fail(s), and 0 exception(s). View

Reroll.

alexpott’s picture

Status: Needs work » Needs review
FileSize
63.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,916 pass(es). View
+++ b/core/modules/user/user.module
@@ -1142,6 +1142,7 @@ function user_role_grant_permissions($rid, array $permissions = array()) {
     $role->save();
   }
+  $role->trustData()->save();

@joshtaylor These lines needed to be merged.

alexpott’s picture

@tim.plunkett and @bojanz actaully if the ConfigEntityType does not provide an annotation it falls back to using schema - also if you do provide a mapping you don't have to provide a schema. So in someways this patch increases flexibility and developer choices.

alexpott’s picture

Priority: Major » Critical

With this patch

Standard site install using drush

real	0m15.207s
user	0m12.212s
sys	0m0.685s

Viewing node/1 on a code cache

XHprof overall summary:

Total Incl. Wall Time (microsec): 1,420,066 microsecs
Total Incl. MemUse (bytes): 60,766,480 bytes
Total Incl. PeakMemUse (bytes): 61,357,280 bytes
Number of Function Calls:	 543,333

Without this patch

Standard site install using drush

real	0m25.119s
user	0m21.334s
sys	0m0.897s

Viewing node/1 on a code cache

XHprof overall summary:

Total Incl. Wall Time (microsec): 2,055,808 microsecs
Total Incl. MemUse (bytes): 62,141,936 bytes
Total Incl. PeakMemUse (bytes): 62,732,288 bytes
Number of Function Calls:	 1,053,130

Given the 10 second improvement on standard install and the 0.5 sec improvement on cold cache I think this should be critical. The reason for the node/1 cold cache performance boost is the entity displays a cached and in building their cache entry they use toArray() which in HEAD requires schema and in this patch does not.

yched’s picture

Wondering what triggers a cache/serialization of the EntityDisplay in node/1 ?
[edit: sorry, I misread @alexpott's comment. More on that in a new comment below]

Also - that number on the cost of toArray() using schema seems scary...

alexpott’s picture

FileSize
1.68 KB
64.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,906 pass(es). View

@Fabianx noticed that we hadn't quite finished removing config_schema_test.schema_in_install

New comparison available here... This shows cold cache node/1 - the improvement has dropped to a 17% reduction in wall time - still significant. Xhprof is unreliable with this many function calls :(. But even with that this is still critical because that level of improvement is still a massive 118ms.

joshtaylor’s picture

Seeing some nice speed improvements when installing via drush as well..

Without:

real	0m18.477s
user	0m13.569s
sys	0m0.197s

With:

real	0m12.381s
user	0m7.610s
sys	0m0.277s
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great now!

Fabianx’s picture

Issue summary: View changes

Added a proposed commit message.

yched’s picture

Also, added some thoughts based on this in #1977206-36: Default serialization of ConfigEntities. Opinions welcome :-)

alexpott’s picture

Ticking the boxes in the commit credit form to make @Fabianx's suggested commit message the commit message.

yched’s picture

The reason for the node/1 cold cache performance boost is the entity displays a cached and in building their cache entry they use toArray() which in HEAD requires schema and in this patch does not

I initially understood that as an EntityViewDisplay being serialized, which felt odd for node/1.

But actually that's EntityManager::getViewModes() - its data is cached, but on cold cache this internally (getAllDisplayModesByEntityType()) calls toArray() on every view_mode / form_mode config entity.
That's not specific to node/1, it would happen on any page after a cache rebuild, when rebuilding local tasks data (form modes / view modes add local tasks in Field UI screens)

getAllDisplayModesByEntityType() calling toArray() on every view_mode / form_mode config entity feels a bit weird, but I don't really have a better proposal. It's really the cost of the schema-based toArray() that is scary :-/

alexpott’s picture

re #70/#75
@yched the cache of the EntityDisplay is caused by calling Drupal\Core\Entity\EntityManager::getAllDisplayModesByEntityType which is called as a part of Drupal\node\NodeViewBuilder::getBuildDefaults.

@yched and yep using schema for toArray() has always been a bit scary because of toArray()'s reliance on schema being available.

Berdir’s picture

More numbers:

PHP 5.6, HEAD:

real	0m22.804s
user	0m20.604s
sys	0m0.372s

PHP 5.6, patch:

real	0m15.362s
user	0m13.284s
sys	0m0.328s

PHP 7, HEAD:

real	0m10.654s
user	0m8.496s
sys	0m0.296s

PHP 7, patch:

real	0m7.357s
user	0m5.352s
sys	0m0.280s
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -173,4 +188,35 @@ protected function checkStorageClass($class) {
    +        $properties_for_annotation = $this->config_export;
    

    $properties_for_annotation is used exactly once, why not just loop over $this->config_export?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -173,4 +188,35 @@ protected function checkStorageClass($class) {
    +        $this->static_config_export = array_unique(array_merge($defaults, $properties_to_merge));
    

    this is a bit weird, because it's not actually static ;)

    Also wondering if we shouldn't camelCase it, to make it clear that it's not an annotation property?

    $this->normalizedPropertyMap or something?

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -173,4 +188,35 @@ protected function checkStorageClass($class) {
    +    return FALSE;
    

    why false and not null? in other places, like entity load, we switched from returning false to null.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

So this will need a reroll for #2204697: Move getConfigPrefix() to ConfigEntityTypeInterface apparently -- also, looks like some feedback in #79 to address.

alexpott’s picture

Status: Needs work » Needs review
FileSize
62.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,928 pass(es). View

Addressed everything in #79 and done the reroll due to #80. Interdiff is tricky due to reroll. C&p of the relevant code that addresses #79.

  /**
   * {@inheritdoc}
   */
  public function getPropertiesToExport() {
    if (!empty($this->config_export)) {
      if (empty($this->mergedConfigExport)) {
        // Always add default properties to be exported.
        $this->mergedConfigExport = [
          'uuid' => 'uuid',
          'langcode' => 'langcode',
          'status' => 'status',
          'dependencies' => 'dependencies',
          'third_party_settings' => 'third_party_settings'
        ];
        foreach ($this->config_export as $property => $name) {
          if (is_numeric($property)) {
            $this->mergedConfigExport[$name] = $name;
          }
          else {
            $this->mergedConfigExport[$property] = $name;
          }
        }
      }
      return $this->mergedConfigExport;
    }
    return NULL;
  }

No weird local variables anymore :)

Berdir’s picture

Yes, getPropertiesToExport() looks much better now :)

Is it worth to have a change record for this? It's kind of new, but also something that existing entity types are expected to implement and possibly also custom hook_install() code.

tim.plunkett’s picture

FileSize
65.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,931 pass(es), 1 fail(s), and 0 exception(s). View
5.76 KB
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -265,20 +272,26 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    +      $definition = $this->getTypedConfig()->getDefinition($config_name);
    +      if (!isset($definition['mapping'])) {
    +        throw new SchemaIncompleteException(SafeMarkup::format('Incomplete or missing schema for @config_name', array('@config_name' => $config_name)));
    +      }
    +      $properties_to_export = array_combine(array_keys($definition['mapping']), array_keys($definition['mapping']));
    

    What's to stop us from moving this into getPropertiesToExport(), in order to avoid recalculation of this as well?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
    @@ -35,6 +35,22 @@ class ConfigEntityType extends EntityType implements ConfigEntityTypeInterface {
    +  protected $config_export = [];
    ...
    +  protected $mergedConfigExport = [];
    

    Why camelCase for one and not the other? Just because one is intended for use in an annotation?

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
    @@ -61,4 +61,13 @@
    +   * @return array|NULL
    

    In the calling code we don't differentiate between [] and NULL (by using empty()), why not just always return an array?

Adding some unit tests to cover the changes to config entities.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
65.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,935 pass(es). View
948 bytes

My apologies. I shouldn't refactor right before uploading without rerunning tests :)

Berdir’s picture

2. Yes, that's exactly why I suggested camel case. It makes sense IMHO to separate it from the annotation keys, because setting it could have a very weird effect. But it's not too important if others disagree.

Berdir’s picture

On 3. I kind of like the explicit definition of not knowing the properties to export. But if we do 1. (and I kind of like it), then that should make this irrelevant because we always would have an array.

alexpott’s picture

The problem with (1) is that we then need to provide the full name - since without that we can't determine the schema. So doing this would make yched sad - see #1977206: Default serialization of ConfigEntities

yched’s picture

Re "1": so yeah, #1977206: Default serialization of ConfigEntities will need to determine cases where ConfigEntityBase ::serialize() can safely call toArray() and be sure that it won't rely on config schema.

AFAICT, the easiest way will be to check for the presence of a config_export key directly, so I'm not sure we will really use getExportProperties(), we have no actual use for its return value anyway, what we need is the end result of toArray().

That would mean ::serialize() makes assumptions on how ::toArray() works (config_export key present == toArray() won't rely on the schema), but I don't really see a better option ?

yched’s picture

In fewer words :-), I don't think I see an issue with getExportProperties() internalizing the "config_export key or use config schema" logic ?

yched’s picture

alexpott’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
@@ -61,4 +61,13 @@
+  /**
+   * Gets the config entity properties to export if declared on the annotation.
+   *
+   * @return array|NULL
+   *   The properties to export or NULL if they can not be determine from the
+   *   config entity type annotation.
+   */
+  public function getPropertiesToExport();

I'd still rather not unnecessarily tie this method to schema and the configuration entity's configuration object name.

alexpott’s picture

Issue summary: View changes
Issue tags: -Needs profiling

We've done the necessary profiling see #69, #72 and #79

alexpott’s picture

FileSize
3.76 KB
68.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,173 pass(es), 3 fail(s), and 0 exception(s). View

Added some tests which found some edge cases in Config and Configuration entities if schema has changed the underlying data.

alexpott’s picture

Issue summary: View changes

Updating suggested commit message.

Status: Needs review » Needs work

The last submitted patch, 94: 2432791.94.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
70.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,169 pass(es). View

Fix up the tests.

Fabianx’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Back to RTBC, looks great now!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed e0aae8c on 8.0.x
    Issue #2432791 by alexpott, vijaycs85, tim.plunkett, joshtaylor, Fabianx...
tim.plunkett’s picture

Wow that was fast. Guess #83.1 and #83.3 didn't make it :(

Fabianx’s picture

#101: Sorry, I missed that it was not yet answered.

Can we do #83.1 as a non-critical follow-up?

catch’s picture

I also missed that it wasn't answered, seems OK for a follow-up though indeed.

vijaycs85’s picture

alexpott’s picture

#92 is my response to #83.1 and .3. I just don't think it is necessary to couple ConfigEntityType to the configuration object name of a Config entity object.

Status: Fixed » Closed (fixed)

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