Problem/Motivation

$this->configuration['fields'] in FieldsProcessorPluginBase is an unassociative array of field names.

FieldsProcessorPluginBase::testField() checks isset($this->configuration['fields'][$field_name]. Because the field names are the values and not the keys, and the values are always integers, this will never return TRUE. Because of this processor plugins will never actually be run.

Proposed resolution

Make testField() check in_array($field_name, $this->configuration['fields'] instead.

Remaining tasks

User interface changes

API changes

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value : 1
Story Points: 3

CommentFileSizeAuthor
#10 2490804-10--fields_processor_testField_test.patch731 bytesdrunken monkey
#10 2490804-10--fields_processor_testField_test--tests_only.patch1.26 KBdrunken monkey
#1 2490804-1-field-processor-plugin-base--test-field.patch572 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer and at erdfisch commented
Status: Active » Needs review
FileSize
2490804-1-field-processor-plugin-base--test-field.patch572 bytes

Here we go. This fixes the problem on my setup.

The IgnoreCase preprocessor failed to work before and now it works like a charm.

Status: Needs review » Needs work

The last submitted patch, 1: 2490804-1-field-processor-plugin-base--test-field.patch, failed testing.

rosinegrean’s picture

rosinegrean CreditAttribution: rosinegrean commented

I've tested your patch and indeed it's working.
I also ran all the search related tests and none failed, not sure why for you 7 failed.
I'll re-test the patch and see what happens.

Status: Needs work » Needs review

prics queued 1: 2490804-1-field-processor-plugin-base--test-field.patch for re-testing.

Alumei’s picture

Alumei CreditAttribution: Alumei commented

Works for me as well.

rosinegrean’s picture

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

I think this is ok like it is now.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Component: Framework » Tests
Priority: Major » Normal
Status: Reviewed & tested by the community » Active
Issue tags: +Needs tests

Looks good, thanks!
Committed.

It's not good that we're currently not catching this with the testing framework, though. Would be great if someone could create some tests for that. (Best integrate it into the IntegrationTest, but even a small sanity check for the testField() method directly in a PhpUnit test would be great.)

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh at Acquia commented
Issue summary: View changes
Issue tags: +beta blocker
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Active » Needs review
FileSize
2490804-10--fields_processor_testField_test--tests_only.patch1.26 KB
2490804-10--fields_processor_testField_test.patch731 bytes

Since we're not doing any indexing or searching with closer checks yet in IntegrationTest, adding it there would be pretty complicated. Fixing the existing FieldsProcessorPluginBaseTest, on the other hand, is trivial.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Title: FieldsProcessorPluginBase::testField() is broken » Add proper tests for FieldsProcessorPluginBase::testField()
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Issue tags: -Needs tests

The last submitted patch, 10: 2490804-10--fields_processor_testField_test--tests_only.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs review » Needs work

The test-only patch here doesn't contain only tests. The other one does. That doesn't seem right?

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

The test-only patch here doesn't contain only tests. The other one does. That doesn't seem right?

That's because the fix was already committed. The "tests only" patch reverts that fix, the other one doesn't.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs review » Reviewed & tested by the community

Oh, that makes sense.

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

Good to hear, thanks for your review!
Committed.

  • drunken monkey committed a0ba9b5 on 8.x-1.x 
    Issue #2490804 by drunken monkey: Added proper tests for...

Status: Fixed » Closed (fixed)

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