WIP: V3 - refactoring #7

[pionl]

• upgrade code to PHP 8.0 with strict types
• reactor with automatic downgrade script
• phpstan 8 + phpcs fixer
• Upgrade queries / aggregations to use strict constructors
• Add GeoBoundingBoxQuery
• Add HistogramAggregation / WidthHistogramAggregation
• Add collapse support
• Improve sub aggregation on aggregations

[pionl]

Started refactoring my code base. At this moment most of the time is rewriting Filters to Query. Its not bad and its quite easy and the code looks pretty now :) 👍

  new BoolQuery(should: [
                new BoolQuery(filter: [
                    new RangeQuery(field: PriceTermsIndex::FROM, gte: $this->from->toDateString()),
                    new RangeQuery(field: PriceTermsIndex::TO, lte: $this->to->toDateString()),
                ]),
                // If the current year has no price set we want to return it as there was no price (will work if
                // HasNumberOfNights->allowTermsWithZeroNights is true)
                new TermQuery(PriceTermsIndex::NUMBER_OF_NIGHTS_RAW, 0),
            ]),

Aggregations were easy.

[erichard]

This is great ! Thanks 🙏

IMHO the code is even prettier when using the static factory ;)

[pionl]

Need to rename the branch from v3 (composer will not allow me to reference branch name with v in it). Do you know how to do it? Otherwise I will have to make a new PR.

Is the code ok for you? (the direction). Am I missing something?

[erichard]

You can rename your branch with git branch -m v3 refact-v3

The direction seems OK to me but he PR is quite large to review. Maybe you can split in mutliple PRs ?
Like one for PHP 8.1 migration, one for rector, one for each new Query, etc

[pionl]

I did rename it but the PR got close but cant edit the PR branch.

About the code review - not sure if I can do it as proposed. It is mixed because rector / phpcs fixer improved the code quality.

• I can split the "proof of concept" of 7.4 / 7.2 code generation.
• I can split the new Query / aggregation (but don't see the time savings on code review).

Any way, how migrate my codebase (apx 50 filters -> queries) and it works 👍

[pionl]

I think im near to finishing all changes, I've made some tweaks I needed to be able to fully customize / use query builder.

I can try to separate the changes.

[erichard]

@pionl I've started to review this PR. One question so far : in the constructors sometime you set a private property and othertime a public one. Is there a particular reason for having both ? Shouldn't we use only private properties ?

[erichard]

Can you add the Symfony ruleset please 🙏

[pionl]

Sure, I thought has this setup for Symfony related stuff but I will check it 👍 Thanks for your time! I will try this week to split it as requested :)

[pionl]

Do you want me to leave the 3 rules?

I've switched to symfony and it made change in yoda_style.

Is this what you want?

-        if (empty($this->innerHits) === false) {
+        if (false === empty($this->innerHits)) {
             $result['inner_hits'] = array_map(fn (InnerHit $hit) => $hit->build(), $this->innerHits);
         }

[erichard]

Yes it's perfect. I work with yoda condition for so long I can't go back !

But in your exemple I can't figure why you don't use a simple if (!empty($this->innerHits)). The empty function only return bool so you don't have to be strict.

[pionl]

Yeah, in our team we are do like to always use a strict comparison even for bools. We find it more readable than finding ! symbol at the start

[pionl]

@pionl I've started to review this PR. One question so far : in the constructors sometime you set a private property and othertime a public one. Is there a particular reason for having both ? Shouldn't we use only private properties ?

Public - I'll check which one and try to find usage. I think I've needed them in some cases (I'm doing some additional logic -> when this aggregation is used do this with source, etc).

Maybe we can expose all to public? Support set/get + "public property" ? Or use set/get everywhere? I can make the changes with your preference.

Thank you, Martin

[erichard]

Maybe we can expose all to public? Support set/get + "public property" ? Or use set/get everywhere? I can make the changes with your preference.

I prefer all privates with getter/setter, I think users will expect to have theses.

[erichard]

I can try to separate the changes.

No need to separate anymore, I reviewed the whole thing already :)

[pionl]

I prefer all privates with getter/setter, I think users will expect to have theses.

Ok I'll make the changes :)

[pionl]

Hi @erichard, I've made the changes, I've improved the git commits and added new StatsAggregation (this commit)

Anything else?

[pionl]

Also I've moved the "downgrade" proof of concept to different branch

[erichard]

Great work ! Thanks @pionl

[pionl]

Thank you for the cooperation!