ConfigStorageController::importChange() doesn't work for protected properties.
Tests and fix coming tomorrow.

CommentFileSizeAuthor
#11 config-1889854-10-FAIL.patch2.51 KBtim.plunkett
#11 config-1889854-10-PASS.patch3.28 KBtim.plunkett
#11 interdiff.txt3.54 KBtim.plunkett
#7 config-1889854-7.patch4.13 KBtim.plunkett
#7 interdiff.txt2.22 KBtim.plunkett
#5 config-1889854-5-FAIL.patch3.51 KBtim.plunkett
#5 config-1889854-5-PASS.patch4.27 KBtim.plunkett
#5 interdiff.txt1.89 KBtim.plunkett
#3 config-1889854-2-FAIL.patch2.65 KBtim.plunkett
#3 config-1889854-2-PASS.patch3.41 KBtim.plunkett
#1 config-1889854-1-FAIL.patch1 KBtim.plunkett
#1 config-1889854-1-PASS.patch1.77 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
config-1889854-1-PASS.patch1.77 KB
config-1889854-1-FAIL.patch1 KB

Okay, this needs a test using the config_test config entity, so I'll work on that, but at least I have something.

Status: Needs review » Needs work

The last submitted patch, config-1889854-1-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Assigned: tim.plunkett » Unassigned
Priority: Normal » Major
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
config-1889854-2-PASS.patch3.41 KB
config-1889854-2-FAIL.patch2.65 KB

Oh, that actually wasn't that hard.

This is a pretty big one, since Views cannot be updated at all.

Status: Needs review » Needs work

The last submitted patch, config-1889854-2-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.89 KB
config-1889854-5-PASS.patch4.27 KB
config-1889854-5-FAIL.patch3.51 KB

Had to adjust the expectations of other tests, and reorder the properties.

Status: Needs review » Needs work

The last submitted patch, config-1889854-5-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.22 KB
config-1889854-7.patch4.13 KB

Ahh, had to adjust for the config entity query commits.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Reviewed & tested by the community

eyup

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
@@ -128,4 +129,22 @@ function prepareSiteNameUpdate($new_site_name) {
+  /**
+   * Tests updating existing configuration.
+   */
+  public function testUpdate() {
+    // Create a staged version of the configuration object with different data.

Why is this part of the UI test?

Let's move it into the API import test.

+++ b/core/modules/config/tests/config_test/config/config_test.dynamic.default.yml
@@ -1,2 +1,3 @@
+description: Default

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigTest.php
@@ -62,4 +62,25 @@ class ConfigTest extends ConfigEntityBase {
+  protected $description;

Can we rename this property to 'protected_property', please?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Also, can we make sure to check for existing issues first? :)

Marked #1892558: Fatal error when importing a view, using set() rather than accessing properties directly might work as duplicate.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.54 KB
config-1889854-10-PASS.patch3.28 KB
config-1889854-10-FAIL.patch2.51 KB

It turned out we already had test coverage for this in ConfigImportTest, and my addition to ConfigImportUITest was redundant (I put it in there initially because I was writing test coverage to mirror my interaction in the UI).

I also did s/description/protected_property, that's a nice clarification.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

That was tagged neither Configurables nor VDC, I can't look everywhere.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Configurables with exported protected properties cannot react to config sync » Config import breaks on protected entity properties
Status: Needs review » Reviewed & tested by the community

Thanks, looks great. :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, config-1889854-10-PASS.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community

Retested through qa.d.o

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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