kirby icon indicating copy to clipboard operation
kirby copied to clipboard

New `Query` classes

Open distantnative opened this issue 1 year ago • 2 comments

This PR …

Features

  • New Kirby\Query namespace with new Kirby\Query\Query class (to be used going forward for queries)
  • New default entries/functions for queries
    • kirby, collection("your-collection"), page("blog"), file("blog/test.pdf"), site, user("homer"), t("your.i18n.key") https://github.com/getkirby/backlog/issues/81
  • Support for closures as query arguments: site.myLazyFunction(() => site.children) - no support for parameters or others, only as lazy-calling measure

Deprecated

  • Old Kirby\Toolkit\Query has been deprecated and will be removed in Kirby 3.9.0. Use Kirby\Query\Query instead, e.g.Query::factory($query)->resolve($data).
  • new Query($query, $data): Passing $data to constructor will be deprecated in a future version. Use (new Query($query))->resolve($data) instead.
  • Query::result() will be deprecated in a future version. Use $query->resolve($data) instead.

Ready?

  • [x] Unit tests for fixed bug/feature
  • [x] In-code documentation (wherever needed)
  • [x] Tests and checks all pass

For review team

distantnative avatar Sep 04 '22 09:09 distantnative

Tweaked the PR, so we should be able to add it in 3.8.x without breaking changes.

distantnative avatar Sep 16 '22 07:09 distantnative

Changed it to keep Toolkit\Query around as deprecated. That way we have no breaking change.

distantnative avatar Sep 21 '22 09:09 distantnative

Since this is quite a massive change, I feel like we should move this to 3.8.2 to reduce the risk of regressions in 3.8.1 (which is supposed to fix regressions and bugs from 3.8.0).

lukasbestle avatar Oct 09 '22 20:10 lukasbestle

The classes and tests look really good. Is there anything in particular that we can do to test it some more? Are there any parts that you are not 100% sure about?

bastianallgeier avatar Oct 28 '22 12:10 bastianallgeier

I think I reimplemented all unit tests of the previous class.

I wasn't 100% sure where to put the inception possibility. But since the Query class would be the most likely one to extend, e.g. for KQL, I built it so that a class method in there gets called.

Also maybe to think about whether we want to have those standard entries/functions (for page, file...) in there.

distantnative avatar Oct 28 '22 12:10 distantnative

I think it would be super nice to add page and file to entries. It is really making query strings a lot easier.

bastianallgeier avatar Oct 31 '22 10:10 bastianallgeier

I would suggest to merge it like it is and then work on enhancements later if we need to.

bastianallgeier avatar Oct 31 '22 11:10 bastianallgeier

The entries are already in place 👍

But agreed, happy to merge it.

distantnative avatar Oct 31 '22 11:10 distantnative