Config entity in save assumed as updated when it's new

Probably there's no test, got this issue working on #1552396: Convert vocabularies into configuration
When vocabulary saved it's always displayed message about Update on new vocabulary

CommentFileSizeAuthor
#10 drupal-config-1789722-10.patch2.05 KBtim.plunkett
#6 drupal-config-1789722-6.patch2.31 KBtim.plunkett
#4 1789722-core-save-4-test.patch1.34 KBandypost
#4 1789722-core-save-4.patch2.42 KBandypost
#2 1789722-core-save-2.patch1.08 KBandypost
#1 1789722-core-save.patch994 bytesandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
Issue tags: -Needs tests
StatusFileSize
new 1789722-core-save.patch994 bytes

Simple fix

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1789722-core-save-2.patch1.08 KB

Finally enforceNew() is moved too

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Really? how on earth can we not need tests for this? The logic was completely bass-ackwards.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1789722-core-save-4.patch2.42 KB
new 1789722-core-save-4-test.patch1.34 KB

Changed a test to get this trouble

Attached 1789722-core-save-4-test.patch should fail

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -283,18 +283,18 @@ class ConfigStorageController implements EntityStorageControllerInterface {
+      $entity->enforceIsNew(FALSE);

I'm curious if this passes tests without this one line?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Critical » Major
StatusFileSize
new drupal-config-1789722-6.patch2.31 KB

D'oh.

Status: Needs review » Needs work

The last submitted patch, drupal-config-1789722-6.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Major » Critical
Status: Needs work » Needs review

I don't fully understand that, I'll have to look at the tests tomorrow. Back to CNR for #4. Also, I didn't mean to change the priority.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: Unassigned » andypost

This is a simple bug and there's already a test - Use patch from #4
let's get this in and continue with conversion to ConfigEntity patches

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new drupal-config-1789722-10.patch2.05 KB

DatabaseStorageController does the same thing, but in the reverse order. Here's a patch that is identical to #4 but with the order of the conditionals matching DatabaseStorageController.

I'd say this is RTBC.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

+1 RTBC and -1 to criticals, btw we should minimize commit thresholds to unfreeze commit features

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +Configuration system

Looks good to me.

webchick’s picture

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

Awesome!!! :D Thanks so much for the quick turnaround on this.

Committed and pushed to 8.x.

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