Skip to content

Conversation

@Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented Jan 13, 2023

Question Answer
JIRA Ticket N/A
Versions 3.3, 4.x

Added JSON sample for /views route, similar to the XML one.

Additional changes

Markdown library and Twig extension has been added to facilitate writing longer descriptions for routes, including paragraphs and links specifically.

Checklist

  • Text renders correctly
  • Text has been checked with vale
  • Description metadata is up to date
  • Redirects cover removed/moved pages
  • Code samples are working
  • PHP code samples have been fixed with PHP CS fixer
  • Added link to this PR in relevant JIRA ticket or code PR

"offset": 0,
"SortClauses": {
"ContentName": "ascending"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have the contentTypeFacetBuilder like in the XML example?

Copy link
Contributor

@MagdalenaZuba MagdalenaZuba Feb 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @Steveb-p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the thing is, Facet Builders were supposed to be removed in 4.x. While I can construct a query in JSON that creates those facet builder objects internally, I'm not sure if they will work properly.

When reading through the code related to search engine (Ibexa\Contracts\Core\Repository\SearchService::findContent) facet builders appear to be unused and therefore silently discarded.

Needs confirmation from @alongosz / @Nattfarinn / @adamwojs: The first two placed the deprecation notice, while Adam is knowledgeable regarding how facet builders / aggregations should behave in context of REST API, or at least could point us in the right direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the thing is, Facet Builders were supposed to be removed in 4.x. While I can construct a query in JSON that creates those facet builder objects internally, I'm not sure if they will work properly.

When reading through the code related to search engine (Ibexa\Contracts\Core\Repository\SearchService::findContent) facet builders appear to be unused and therefore silently discarded.

Needs confirmation from @alongosz / @Nattfarinn / @adamwojs: The first two placed the deprecation notice, while Adam is knowledgeable regarding how facet builders / aggregations should behave in context of REST API, or at least could point us in the right direction.

Aggregations should be used instead of Facet builders.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz That is plain. I'll provide concrete questions:

  1. Do facet builders still work in context of REST API?
  2. Do aggregation work in context of REST API (as I haven't found direct reference to their property in ViewInput class yet)?
  3. Do you know the structure of XML/JSON that should be used for REST API to make it work (I'm looking for tests that might cover this case and / or directly looking into code at this very moment, but maybe you have some samples on hand)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz That is plain. I'll provide concrete questions:

  1. Do facet builders still work in context of REST API?
  2. Do aggregation work in context of REST API (as I haven't found direct reference to their property in ViewInput class yet)?
  3. Do you know the structure of XML/JSON that should be used for REST API to make it work (I'm looking for tests that might cover this case and / or directly looking into code at this very moment, but maybe you have some samples on hand)?

It doesn't matter whether it works or not, it shouldn't be advocated in the doc.

To fully answer these questions 1 and 3 I need to spend significant amount of time, so you'll need to wait once I have such time slot available. Question 2 is for Adam, I have no knowledge of Aggregations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz Then do not worry about 1 & 3. We will simply drop the FacetBuilders property from the REST API doc and I'll look into the structure for aggregations, as I have all the files related to REST open atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adriendupuis added samples with Aggregations, and added a reference to our Search Criteria reference page - to do this I also added markdown dependency on the raml builder tool (which I think would be a nice improvement over trying to squeeze pure HTML into method description, which is also an option).

A few things I've discovered when testing those, so I'll be pushing PRs into rest package, and cross-link them here.

@Steveb-p Steveb-p force-pushed the add-views-search-sample branch from 9cc8fda to 8972ca1 Compare February 6, 2023 11:31
@MagdalenaZuba MagdalenaZuba merged commit 53e4989 into 3.3 Feb 9, 2023
@MagdalenaZuba MagdalenaZuba deleted the add-views-search-sample branch February 9, 2023 09:47
MagdalenaZuba pushed a commit that referenced this pull request Feb 9, 2023
MagdalenaZuba pushed a commit that referenced this pull request Feb 9, 2023
MagdalenaZuba pushed a commit that referenced this pull request Feb 9, 2023
"ramsey/uuid": "^3.9"
"ramsey/uuid": "^3.9",
"twig/markdown-extra": "^3.5",
"league/commonmark": "^2.3"
Copy link
Contributor

@adriendupuis adriendupuis Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires league/commonmark ^2.3 -> satisfiable by league/commonmark[2.3.0, ..., 2.3.8].
    - league/commonmark[2.3.0, ..., 2.3.8] require php ^7.4 || ^8.0 -> your php version (7.2.34) does not satisfy that requirement.

So, this addition is not fully compatible with "php": "^7.2", above.

Copy link
Contributor

@adriendupuis adriendupuis Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick fix in c34be1469801e0ce0035448577074fbbe991a4fa / #1642 for PHP 7.4 w/ few warnings at runtime.

A better fix for PHP 8.1+ will come in #1871

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adriendupuis we will need to take the raml parser and everything else related to it under our organization, because that library is in practice abandoned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants