Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement EZP-23247: missing field type Indexable definitions #1418

Merged
merged 9 commits into from Sep 24, 2015

Conversation

3 participants
@pspanja
Copy link
Contributor

pspanja commented Sep 21, 2015

This PR resolves https://jira.ez.no/browse/EZP-23247

Review with ezsystems/ezplatform-solr-search-engine#21

This implements Indexables for XmlText and RichText field types.
Previously these were handled by TextLine implementation of Indexable.

Implementing this properly required separately indexing field type value as string field and as full text field.
String field indexes shortened variant of the field value, while full text field indexes complete text content of the field value. This is achieved by using new SPI search field type FullText. Fields of this type, in both Solr and Elasticsearch, are not stored, but instead copied to the real full text field.
This gives better control of what goes into full text index than was the case previously. Explicit copying of all text fields (string, text and HTML) to the full text field is removed.

Existing field types are updated to use new FullText search backend field type.

Additionaly fixed

  • removed handling of DOMDocument from string field value mapper (cleanup after merge of #262)
  • Relation field type's Indexable definition was passing an array of strings

TODOs

  • Add new cases to field type search tests

@pspanja pspanja force-pushed the impl-EZP-23247-indexable-xml branch from f4bd687 to e8a1f1c Sep 21, 2015

@pspanja

This comment has been minimized.

Copy link
Contributor Author

pspanja commented Sep 21, 2015

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Sep 21, 2015

+1

But any test coverage?

$text .= $node->nodeValue . ' ';
}
return $text;

This comment has been minimized.

Copy link
@lolautruche

lolautruche Sep 22, 2015

Contributor

Couldn't we just do a strip_tags() on string representation? 😜

This comment has been minimized.

Copy link
@pspanja

pspanja Sep 22, 2015

Author Contributor

Not if we don't want concatenated strings, it would require some preprocessing to avoid that.

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Sep 22, 2015

+1

@pspanja

This comment has been minimized.

Copy link
Contributor Author

pspanja commented Sep 22, 2015

@andrerom

But any test coverage?

Some tests with FullText criterion, but we should have something in the field type test framework.
Todo added.

@pspanja pspanja force-pushed the impl-EZP-23247-indexable-xml branch from 2be1d3b to c8ff380 Sep 22, 2015

@pspanja

This comment has been minimized.

Copy link
Contributor Author

pspanja commented Sep 22, 2015

Implemented full text field type tests, re-review ping for last commits.

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Sep 22, 2015

+1

@pspanja pspanja force-pushed the impl-EZP-23247-indexable-xml branch from 2c68f27 to 5f8d787 Sep 24, 2015

andrerom added a commit that referenced this pull request Sep 24, 2015

Merge pull request #1418 from ezsystems/impl-EZP-23247-indexable-xml
Implement EZP-23247: missing field type Indexable definitions

@andrerom andrerom merged commit cc4c02a into master Sep 24, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ezrobot Code review by ezrobot
Details

@andrerom andrerom deleted the impl-EZP-23247-indexable-xml branch Sep 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.