Problem/Motivation

With a combination of processors the content of 'Rendered Item' was being removed and replaced with an empty array. The value of Rendered Item was originally a string, later an array of items, which tokenizer was overwriting.

Proposed resolution

Rather than the value being set as a string it should be handled by the text datatype handler.

Suggestion to move the data type handing code from Utility::extractField() to Field::addValue().

Remaining tasks

Do it.

User interface changes

None - except for less fatal error :)

API changes

None

CommentFileSizeAuthor
#14 2752963-14--data_type_value_processing.patch9.18 KBdrunken monkey
#14 2752963-14--data_type_value_processing--tests_only.patch4.49 KBdrunken monkey
#14 2752963-14--data_type_value_processing--interdiff.txt386 bytesdrunken monkey
#9 2752963-9--data_type_value_processing.patch9.15 KBdrunken monkey
#9 2752963-9--data_type_value_processing--tests_only.patch4.46 KBdrunken monkey
#9 2752963-9--data_type_value_processing--interdiff.txt7.89 KBdrunken monkey
#6 2752963.06.item_type_value.patch3.64 KBekes
#2 2752963.02.item_type_value.patch1.87 KBekes
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ekes created an issue. See original summary.

ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented
FileSize
2752963.02.item_type_value.patch1.87 KB

Cut and Paste patch - for tests (seems like it's working, but tests failing locally).
Question, shouldn't the Data Type plugin be loaded with the class?

ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2752963.02.item_type_value.patch, failed testing.

The last submitted patch, 2: 2752963.02.item_type_value.patch, failed testing.

ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented
FileSize
2752963.06.item_type_value.patch3.64 KB

Test fixes.

ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented
Status: Needs work » Needs review
ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented
Issue tags: +drupaldevdays
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue tags: +DevDaysMilan
FileSize
2752963-9--data_type_value_processing--interdiff.txt7.89 KB
2752963-9--data_type_value_processing--tests_only.patch4.46 KB
2752963-9--data_type_value_processing.patch9.15 KB

Great job, thanks a lot for working this out!
The patch already looks quite good. The attached revision includes a bit of clean-up, documentation for the newly defined differences between Field::setValues() and Field::addValue() and some more tests to ensure this doesn't happen again.
Please test/review!

Also thanks for thinking of the tag!

The last submitted patch, 9: 2752963-9--data_type_value_processing--tests_only.patch, failed testing.

The last submitted patch, 9: 2752963-9--data_type_value_processing--tests_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2752963-9--data_type_value_processing.patch, failed testing.

The last submitted patch, 9: 2752963-9--data_type_value_processing.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Needs work » Needs review
FileSize
2752963-14--data_type_value_processing--interdiff.txt386 bytes
2752963-14--data_type_value_processing--tests_only.patch4.49 KB
2752963-14--data_type_value_processing.patch9.18 KB

The last submitted patch, 14: 2752963-14--data_type_value_processing--tests_only.patch, failed testing.

The last submitted patch, 14: 2752963-14--data_type_value_processing--tests_only.patch, failed testing.

ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented

The handling of the Datatype manager in Field class makes a lot more sense, plus clarifying the use of the two set/add methods.

I'm not actually sure how to test that it doesn't happen again. The values were being removed by the set method working correctly, but being passed bad data (an empty array) by the processor.
I assume the theory of the additional test is that If the processors are always handling the typed data (is this now _always_ the case?) it shouldn't happen that the incorrect data gets passed?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented

Thanks for reviewing, good to hear you like the changes!

I assume the theory of the additional test is that If the processors are always handling the typed data (is this now _always_ the case?) it shouldn't happen that the incorrect data gets passed?

I'm just testing more or less exactly what our fix was: we just changed the field to always use the data type plugin for processing when adding values, so as long as that happens correctly this error shouldn't happen again.
If the values are correctly processed by the data type plugin, the correct further handling of the values by the processors is already verified by FieldsProcessorPluginBaseTest.

So, is this RTBC, in your opinion?

ekes’s picture

ekes CreditAttribution: ekes as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Makes sense. So just adding the reason? But for the rest all good.

diff -u b/tests/src/Unit/ItemFieldTest.php b/tests/src/Unit/ItemFieldTest.php
--- b/tests/src/Unit/ItemFieldTest.php
+++ b/tests/src/Unit/ItemFieldTest.php
@@ -66,6 +66,9 @@
   /**
    * Tests adding a value.
    *
+   * Ensures that a string passed to addValue is processed by the data type
+   * plugin.
+   *
    * @covers ::addValue
    */
   public function testAddValue() {

  • drunken monkey committed 64f195a on 8.x-1.x authored by ekes 
    Issue #2752963 by ekes, drunken monkey: Fixed data type handling of...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Sure, makes sense. Thanks!
Amended and committed.
Thanks again for your great work on this!

Status: Fixed » Closed (fixed)

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