For some reason, while we moved the datasource configuration into their own property on the index, we kept the processors inside the options array, making these a bit more awkward to handle. I think moving them to their own, dedicated property, too, would make sense.

We could also think about moving the fields configuration as well. Or is there a specific reason it should be in the options, other than that it was like that in D7 (and I didn't want countless serialized database columns back then – now not an issue anymore)?

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
#27 2317771-27--processors_and_fields_properties.patch37.58 KBdrunken monkey
#27 2317771-27--processors_and_fields_properties--interdiff.txt9.64 KBdrunken monkey
#26 move_processor_and_or-2317771-26.patch36.89 KBborisson_
#22 2317771-22--processors_and_fields_properties.patch58.38 KBdrunken monkey
#22 2317771-22--processors_and_fields_properties--interdiff.txt26.54 KBdrunken monkey
#21 move_processor_and_or-2317771-21.patch58.38 KBborisson_
#21 interdiff.txt2.25 KBborisson_
#20 move_processor_and_or-2317771-20.patch57.84 KBborisson_
#20 interdiff.txt5.72 KBborisson_
#17 move_processor_and_or-2317771-17.patch52.13 KBborisson_
#17 interdiff.txt2.3 KBborisson_
#14 move_processor_and_or-2317771-14.patch52.33 KBborisson_
#14 interdiff.txt35.62 KBborisson_
#13 move_processor_and_or-2317771-13.patch20.2 KBborisson_
#13 interdiff.txt971 bytesborisson_
#9 move_processor_and_or-2317771-9.patch19.25 KBborisson_
#9 interdiff.txt1.2 KBborisson_
#6 move_processor_and_or-2317771-6.patch19.29 KBborisson_
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

drunken monkey’s picture

Project: Search API (8.x) » Search API
Version: » 8.x-1.x-dev
Issue tags: +beta deadline
Nick_vh’s picture

Issue summary: View changes
Issue tags: +beta blocker
drunken monkey’s picture

We should probably do this ASAP, to break as few installations as possible. (Since this will break all of them.)

drunken monkey’s picture

The index should then also have setters for both that automatically sort the new settings. Currently, it seems that code is in the form submit handlers, where it of course doesn't belong.

drunken monkey’s picture

OK, not ASAP, apparently.

Anyways, since I'm again struggling with fields and (static) caching: we could then resolve the whole problem by just loading the field objects once for the index, having them as the canonical representation of the fields (basically) and forbidding any direct access to the actual, stored $fields property, and then writing the field objects back to the stored property only when the index is saved.
It seems to me this could resolve a huge load of caching troubles.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 6: move_processor_and_or-2317771-6.patch, failed testing.

The last submitted patch, 6: move_processor_and_or-2317771-6.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 9: move_processor_and_or-2317771-9.patch, failed testing.

The last submitted patch, 9: move_processor_and_or-2317771-9.patch, failed testing.

borisson_’s picture

Assigned: Unassigned » borisson_

Going to move fields as well, will post work later today.

borisson_’s picture

This patch should be green.

borisson_’s picture

This still breaks in a 100 million ways, uploading as intermediate work.

Status: Needs review » Needs work

The last submitted patch, 14: move_processor_and_or-2317771-14.patch, failed testing.

The last submitted patch, 14: move_processor_and_or-2317771-14.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 17: move_processor_and_or-2317771-17.patch, failed testing.

The last submitted patch, 17: move_processor_and_or-2317771-17.patch, failed testing.

borisson_’s picture

borisson_’s picture

Attached patch fixes some small issues I found with my own patch in #20. Those are some small documentation fixes.

I also made the IntegrationTest::addFieldsToIndex a little bit more robust by adding some extra assertions, those aren't directly related to this patch and I can move them into a new issue if that's needed. I think it might be a good thing to add those assertions in more places where we're posting forms / making changes.
+ $this->assertFieldChecked('edit-fields-entitynodetitle-indexed', 'title is saved');

I will probably have another look at this patch tomorrow morning to see if I can find other nitpicks, but most of the work in the patch is find-replace work. The testsuite agrees with the work I did, I'm confident in the work done here.

drunken monkey’s picture

Thanks for implementing this change!

However, I fear the timing isn't great, with #2574969: Add a Views-like UI for adding fields almost finished and changing a lot of the fields code, this is sure to require a lot of re-rolling work either way. I think committing the other one first and then re-rolling this one might be easier, but I'm not sure.

The patch itself looks of course mostly good, just two things:

  1. I think the order "fields, processors, options" makes the most sense for the index properties. But, in any case, this should be uniform across the code.
  2. I don't think getCachedFields() is a good new name for the method, the fact that they are cached should be mostly transparent to callers of the method, and in any case not have any effect on them. I propose getFieldObjects() in the attached patch, but actually I think the solution from the processors makes more sense – using [gs]etFieldSettings() for the new methods and keeping getFields() as-is. In any case, I don't see a reason why fields and processors would have different mechanisms there, this is just bound to lead to confusions.
    What would you say?
borisson_’s picture

[gs]etFieldSettings() does make more sense. Let's implement that as well.

But I agree, let's get #2574969: Add a Views-like UI for adding fields in first.

borisson_’s picture

Status: Needs review » Needs work

back to NW.

borisson_’s picture

Status: Needs work » Postponed
Related issues: +#2574969: Add a Views-like UI for adding fields

Postponing this on that.

borisson_’s picture

Fixes #23. I redid everything in this patch again from scratch because of that change + the field UI commit.

drunken monkey’s picture

Thanks for the re-roll (or, rather, redo)! Looks pretty good, just a few style issues, as usual. (Mostly adjusting method order to my taste.)
The exception when trying to access/change the old options is a pretty good idea. However, we should be sure to remove it before the stable release, I don't think such code should be present there. In the meantime it could help a few people, though.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

The exception when trying to access/change the old options is a pretty good idea. However, we should be sure to remove it before the stable release, I don't think such code should be present there. In the meantime it could help a few people, though.

That was left for my own debugging purposes to make sure I didn't forget converting anything. I think we can leave this in for the benefit of other modules for a while. Until beta release, probably. Let's create an issue about it as well.

Otherwise, happy with the changes.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

As per our IRC discussion, removed the exceptions and committed.
Thanks again for all your work on this!

swentel’s picture

Status: Fixed » Closed (fixed)

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