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

Specified refactoring of facet related APIs #1960

Open
wants to merge 2 commits into
base: master
from

Conversation

@kore
Copy link
Contributor

kore commented Apr 19, 2017

Specified refactoring of facet related APIs

identifier. If a criterion is missing to query that ID we should just
implement this (`SectionIdCriterion`). The queries are supposed to be
more performant this way and it spares us re-indexing of the search
index.

This comment has been minimized.

Copy link
@andrerom

andrerom Apr 25, 2017

Member

.. if identifiers change

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Apr 25, 2017

Some topics that could need some further clarification:

  • some standardization on facet sub types, in case of User this is $type, on Criteria it is often called target
  • labels, missing or covered by Facet Return Values?
    • Solved in ezsystems/ezplatform-solr-search-engine#92, facet->name now comes from what user sets on facetBuilder->names. This can be translatable string, or more applicable an identifier used for translatable string in UI.
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 2, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 4, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 4, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 4, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
@kore

This comment has been minimized.

Copy link
Contributor Author

kore commented May 8, 2017

What do you mean by "labels"? We have the $name in there which could (should?) be used as a label for the facet in the frontend. Do you see labeling required for anything else?

Regarding $type vs. $target: I am all for cleaning things like this up, but seems a bit like an unnecessary BC break. On the other hand: We can deprecate one property and use interceptors to provide a BC. The $target is used in the criteria because of the generic constructor. $type seems like better naming to me, but we should probably go with the more established $target then. Shall I check through the FacetBuilder sub types to see which could use modification?

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented May 8, 2017

Afaik name would be the user defined label for each facet, what I was getting at is to find away to avoid users having to load each and every object present on the facet entries to generate labels for those. However this is clearly not SPI logic, but it might make sense for API to do this work on behalf of the user in a optimal way, it already knows which prioritized languages the user has asked for, and has full access to SPI to enrich the entries.

andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 9, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 9, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 9, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 9, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 10, 2017
1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960
andrerom added a commit to ezsystems/ezplatform-solr-search-engine that referenced this pull request May 22, 2017
* Change Facet visitor logic to be able to inject FacetBuilder

1. To be able to inject name
2. To be able to inject the builder itself eventualy

See planned api improvments on facets:
ezsystems/ezpublish-kernel#1960

* Implementing User facet

* Don't throw on unsupported facet builders

* Enable facets for LocationSearch
andrerom added a commit that referenced this pull request May 24, 2017
…ntation

As in:
- Current implementation in Solr uses id's for all, and API doc was inconsistent
- As argued in #1960 id should be used as identifier would mean we would need to re index if it changes.
### Unsupported Facets

If a storage engine does not support facets it should just ignore them
and not throw a exception. The frontend code should expect that less

This comment has been minimized.

Copy link
@andrerom
$facetBuilderMap[md5(serialize($facetBuilder))] = $facetBuilder;
}

Instead of `serialize()` we might want to use `spl_object_hash()` or something

This comment has been minimized.

Copy link
@andrerom

andrerom Jun 12, 2017

Member

Note: Done in ezsystems/ezplatform-solr-search-engine@d2d4fcb using spl_object_hash() (v1.10, aka v1.4 Solr Bundle)

identifier. If a criterion is missing to query that ID we should just
implement this (`SectionIdCriterion`). The queries are supposed to be
more performant this way and it spares us re-indexing of the search
index if identifiers change.

This comment has been minimized.

Copy link
@andrerom

andrerom Jun 12, 2017

Member

Note: Updated api doc to relfect in 13b710d (v1.10, implementation in Solr was already doing this)

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