Problem/Motivation

Entity reference field configs saved before #2448503: Convert the "Field edit" form to an actual entity form are still containing the stale settings.handler_submit key.

Proposed resolution

Remove the stale 'handler_submit' setting form the ER field configs.

Remaining tasks

  • Write post update hook to fix the ER config.
  • Update path tests.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#21 2715589-21.patch4.24 KBclaudiu.cristea
#21 2715589-21.interdiff.txt3.3 KBclaudiu.cristea
#18 2715589-18.interdiff.txt853 bytesclaudiu.cristea
#18 2715589-18.patch4.31 KBclaudiu.cristea
#16 2715589-16.interdiff.txt2.76 KBclaudiu.cristea
#16 2715589-16.patch4.35 KBclaudiu.cristea
#9 interdiff-4-to-8.txt4.2 KBclaudiu.cristea
#9 2715589-8.patch2.38 KBclaudiu.cristea
#4 don_t_store_the-2715589-4.patch2.57 KBjibran
#4 interdiff.txt1.74 KBjibran
#2 don_t_store_the-2715589-2.patch851 bytesjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Active » Needs review
FileSize
don_t_store_the-2715589-2.patch851 bytes

Here is the patch to start the party.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

The patch looks good, it simply brings back a line of code that was removed by accident.

Would you like to write the upgrade path and maybe the tests as well so we can bring this issue to completion? :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue tags: +Needs upgrade path tests, +Needs tests
FileSize
interdiff.txt1.74 KB
don_t_store_the-2715589-4.patch2.57 KB

Added upgrade path.

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.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Component: entity_reference.module » entity system

Moving to right component

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
+++ b/core/modules/field/field.install
@@ -104,3 +118,30 @@ function field_update_8003() {
+    // Deal only with entity reference fields and descendants.
+    if ($class == EntityReferenceItem::class || is_subclass_of($class, EntityReferenceItem::class)) {

isn't is_subclass_of() enough?

Needs work for a reroll and tests. Otherwise makes sense.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
  • I think the update is more a post_update.
  • Fixed the update function description s/Removes/Remove
  • Fixed some docs/comments.

@Berdir, no, is_subclass_of() doesn't catch the class itself. For example is_subclass_of(EntityReferenceItem::class, EntityReferenceItem::class) returns FALSE.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
FileSize
2715589-8.patch2.38 KB
interdiff-4-to-8.txt4.2 KB

Ouch, the patch!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
Status: Needs review » Needs work

isn't is_subclass_of() enough?

Please don't use it DER extend this class but stores settings differently. NVM said it to soon.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented 
+++ b/core/modules/field/field.post_update.php
@@ -76,3 +77,35 @@ function field_post_update_email_widget_size_setting() {
+  foreach ($config->listAll('field.field.') as $field_id) {

If we are going to use post update hook then we can use field type manager to load the fields by type.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Assigned: Unassigned » claudiu.cristea

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Assigned: claudiu.cristea » Unassigned

I cannot reproduce anymore this bug. All form buttons are removed from the form state values in \Drupal\Core\Entity\EntityForm::submitForm(), specifically in the $form_state->cleanValues();. I think we should keep only the part that updates the DB.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests, -Needs tests
FileSize
2715589-16.patch4.35 KB
2715589-16.interdiff.txt2.76 KB

The issue has been fixed in #2448503: Convert the "Field edit" form to an actual entity form, we are only updating stale settings in field configs.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented 
+++ b/core/modules/field/tests/src/Functional/Update/EntityReferenceFieldConfigCleanUpdateTest.php
@@ -0,0 +1,43 @@
+    //print_r('kasdjhajdfhjshdfjhdsj');

Test looks good there is some debug code left in the patch. Can we please also address #11?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea at Webikon commented
Issue summary: View changes
FileSize
2715589-18.patch4.31 KB
2715589-18.interdiff.txt853 bytes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Thanks, this is ready.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/field/tests/src/Functional/Update/EntityReferenceFieldConfigCleanUpdateTest.php
@@ -0,0 +1,41 @@
+  public function testEntityReferenceFieldConfigCleanUpdate() {

Is there any reason for not putting this test inside \Drupal\Tests\field\Functional\Update\FieldUpdateTest where we have all the other field update tests?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Title: Don't store the 'handler_submit' button value into the ER field config settings » Remove stale 'handler_submit' setting from the ER field configs
Issue summary: View changes
FileSize
2715589-21.interdiff.txt3.3 KB
2715589-21.patch4.24 KB

Ok, ok.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Awesome :)

  • catch committed cf553a0 on 8.5.x 
    Issue #2715589 by claudiu.cristea, jibran, amateescu: Remove stale '...

  • catch committed dbd8fbe on 8.4.x 
    Issue #2715589 by claudiu.cristea, jibran, amateescu: Remove stale '...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2715589-21.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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