Implement unified search provider

[PFischbeck]

This fixes #559 by implementing a search provider for a unified search. Matching recpies are listed with their thumbnail (if available) and title, as well as the category if applicable.

The subline listing the category could easily be replaced by the recipe description, I am open for suggestions here. But I want to note that the current category lookup is expensive, as we read it from the recipe JSON file. Is that okay, or should we handle it differently?

[codecov]

Codecov Report

Merging #611 (6d65c90) into master (ddfea86) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 6d65c90 differs from pull request most recent head a69938d. Consider uploading reports for the commit a69938d to get more accurate results

@@             Coverage Diff             @@
##             master    #611      +/-   ##
===========================================
- Coverage      0.99%   0.97%   -0.03%     
- Complexity      452     453       +1     
===========================================
  Files            15      17       +2     
  Lines          1413    1443      +30     
===========================================
  Hits             14      14              
- Misses         1399    1429      +30     

Flag	Coverage Δ	Complexity Δ
integration	0.00% <0.00%> (ø)	0.00 <1.00> (ø)
unittests	0.97% <0.00%> (-0.03%)	0.00 <1.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
lib/AppInfo/Application.php	0.00% <0.00%> (ø)	1.00 <1.00> (?)
lib/Search/Provider.php	0.00% <0.00%> (ø)	0.00 <0.00> (?)

[PFischbeck]

It fails for Nextcloud 18 and 19, because the interface for unified search was only added in Nextcloud 20. I see no easy way of supporting both pre-20 and 20 in the code.
NC 18 could be dropped, because it is EOL, but would dropping NC 19 be too drastic? We will probably run into the same problem if we tackle #604.

[seyfeb]

I think the category is a good choice for the subline, as descriptions are typically too long be meaningful if only the first few words are shown.

Regarding the category access: Maybe we could add a function for this to the Db service. Categories are stored in the database, so it should improve the performance. @christianlupus certainly has an opinion on this ;)

Regarding older NC support: I would not drop NC 19 support just to have this new feature. However, can’t you check the NC version before registering the service? I’m not to firm in PHP but since it’s not compiled, I guess this should work(?)

[christianlupus]

I think the category is a good choice for the subline, as descriptions are typically too long be meaningful if only the first few words are shown.

Yes, I think this might be best suited at least fo now.

Regarding the category access: Maybe we could add a function for this to the Db service. Categories are stored in the database, so it should improve the performance. @christianlupus certainly has an opinion on this ;)

As I am considering adding a DB abstraction layer that should handle these types of access quickly, you can use this method best. It returns the category from a recipe using only the DB. No need to manually traversing the file tree.

Regarding older NC support: I would not drop NC 19 support just to have this new feature. However, can’t you check the NC version before registering the service? I’m not to firm in PHP but since it’s not compiled, I guess this should work(?)

We had recently a few issues regarding older NC instances (17 (!) and 18 if I remember correctly). So it seems there is quite a user base out there that is not living on the edge. I would rather avoid dropping support for officially supported NC versions (aka 19) yet.
There is a similar issue in #605 that we need to keep compatible with different NC versions. I think, here it might be useful to check the NC version and register only in NC >=20.

[PFischbeck]

Thanks for your suggestions! I now use the method you suggested to retrieve the category from the DB.

I had also thought about checking the NC version before registering the search provider. However, the problem already lies in the definition of the Application class, which implements IBootstrap. If the Application does not implement this interface, the register function will not be called (see documentation here). However, this interface does not exist in NC < 20. Is there a way to poly-fill entire interfaces (IBootstrap, IBootContext, IRegistrationContext)?

Otherwise it seems like we will have to wait until NC 19 is EOL in June 2021... 😞

[christianlupus]

You could try to do something like the following (not sure if it really helps, but maybe worth a try):

<?php
// This is the \OCA\cookbook\AppInfo\Application file
if (/* check if NC < 20 */) {
  namespace OCP\AppFramework\Bootstrap;
  interface IBootstrap {
    /* Specify the interface methods or even nothing just to make the name of the interface known */
  }
}

namespace \OCA\Cookbook\AppInfo;
use OCP\AppFramework\Bootstrap\IBootstrap;
class Application implements IBootstrap { /*...*/ }

[PFischbeck]

Thanks for the suggestion! I have solved it in a slightly different way now, since I couldn't get the conditional definition of the interface in another namespace to work. Instead, I only conditionally define the Application and Provider classes. So this now runs in both NC19 and NC20+!
Should I add a changelog entry myself, or will you do that?

[seyfeb]

You’re welcome to add the Changelog entry yourself :)

[christianlupus]

One more thing I am seeing at the moment:
On NC19 the files are in face empty. no class is defined at all. I do not know if this is good as it could cause trouble if the autoloader scans a file and afterward cannot find the matching class (might be implementation-dependant). I suggest to have an else branch with an otherwise empty class definition.

Apart from that: Good job!

PS: I have a few issues with my dev installation. So I have to test once it is running again. Cannot tell you if I find some bugs directly.

[PFischbeck]

I've made the suggested changes, although the Application definition could not be completely empty.

However, the PHP code style checker failure seems unrelated to my changes. I already have some findings about it: The recent minor update 2.18.4 of php-cs-fixer seems to be not backward-compatible, and we are not the only people hit by this, see this issue. We'll have to wait for a new release, but such problems could be prevented by also committing the composer.lock file to the cookbook repository to pin the dependency versions, as recommended in the Composer documentation. Is there any particular reason why we don't do it this way right now?

[christianlupus]

I was not able to search using this approach. Instead, I got errors in the nextcloud log regarding undefined return values. I stepped through the code and it seems the problem is with the case or IURLGenerator. With the changes mentioned here, I was able to search successfully.

Please revisit these changes and check if you really had the correct case in your dev. Also what version were you developing against? I used a recent NC21 (hopefully, there is no undocumented change) under linux.

[PFischbeck]

You're totally right about IURLGenerator, I applied the suggested changes. I'm running on NC21 via your nextcloud-docker-debug tool, and I'm pretty sure it did work, and I never changed the casing before. Weird. Either way, it should work now.

[christianlupus]

Seems good now to me. I just checked on my test install. Will merge if no issues arise during checks.