Refactor for SOLID compliance - Breaking on getAllIndexes (method return) and Index class (renamed into Indexes)

[ppshobi]

No description provided.

[curquiza]

Hello @ppshobi! Nice to see you again!! 😄
Is this PR ready for review? If it's not, could you convert it as a draft, please? :)

Another question: how does this refactor impact the usage of the package? In other word: what would be the new Getting Started (if there is any changes)? 🙂

[ppshobi]

The getting started would not change at all. Only the getAllIndexes() methods return will have a change. Since it will be returning an array of Index::class objects instead of array of arrays.

Also by making a release-draft what you mean? change the version number in Meilisearch.php?

Ah, sorry. I think this PR is ready to review. Please take a look

[curquiza]

Great!! Thanks a lot!
So cool it does not change the docs and the Getting Started!

But it is a breaking change on getAllIndexes -> this is not a problem, we are going to upgrade the minor of the version 😉

I check your PR on Monday @ppshobi! 🙂

[ppshobi]

Hi @Guikingone , If you can please resolve the conversations as appropriate.

@curquiza The tests are failing due to a recent release from yesterday (25-jun-2020), due to which some endpoints are not returning values as expected. Maybe meiliearch-core have changed the public api, in that case we need to update the tests. Can you confirm the failing tests from the CI logs?

meilisearch/meilisearch#798

[Guikingone]

Hi @ppshobi

For the comments, they're not reviews so I cannot solve them but that's good for me, consider squashing your commits in order to clean the index (except if @curquiza as a special vision on the merged commits 🤔 ) 🙂

[ppshobi]

@Guikingone usually @curquiza sqaush them while merging. Thanks for your comments

[curquiza]

FYI: the tests are currently failing because of:

• this issue
• new MeiliSearch bump (from v0.11.0 to v0.11.1) and this PR fixes the tests. The MeiliSearch behavior regarding the settings changed indeed a little bit.

[curquiza]

Hello guys!!

---

@ppshobi

Thanks again for PR again! I approved it.
Some points:

• I'm not going to merge this PR until MS v0.11.2 is out, that would be done in the day. Then I will merge Update tests after MeiliSearch bump to v0.12.0 #45 and Implement getOrCreateIndex method #47 which are smaller PRs, and finally, I will merge your PR. Merge conflicts would be easier to manage on your side 🙂
• Sorry if I missed something, but I didn't find an answer to my question I asked here:

A question (sorry not to ask it before but I understand better and better each time I read your PR): why do you split the settings in 2 parts? There are:

• one in the index endpoints Endpoints/Indexes.php
• another in the delegate Delegates/handlesSearchSettings.php

Why do you use a delegate for the settings? I would say to separate a group of functionality.
So, why do you separate the general settings functionality from the others? 🙂

---

@Guikingone

Thanks again for your review! I can see you plan to do some changes to this repo 🎉 Feel free to open an issue an start a PR 🙂 Your contribution would be really appreciated!

Also, as you plan to do the symphony integration, here are some useful links:

• the integration guides repository that basically explains the minimal requirements for a new SDK/Integration. You should take a look at it 😉
• our Slack. You can contact me directly on it (I'm Clémentine). I recommend you join us, it's more convenient to work together in this kind of project.
• I've already sent you, but do not forget the already existing laravel-scout package. This package was done and is still mainly maintained by an external contributor 🙂 If you are interested in discussing with him, I will give you his name on our Slack if you join us.

---

Thanks again guys! A huge work has been done here!

[ppshobi]

Great. Thank you :)

So regarding those traits. Previously all these methods were together inside the Index class or HttpRequest Class separated by comments. So initially what I have done is creating traits with names related to their functionality and put related methods in them instead of being separated by comments. And I kept methods as it is to avoid any public api changes.

so the methods inside src/Delegates is related to the overall meilisearch engine. For example managing the indexes, getting the stats, getting the version etc.. But the traits inside the Endpoints are related to that sepecific endpoint for example managing a single index, managing the documents inside and index, managing the displayed-attributes of an index etc... actually these methods belongs to Indexes endpoint but I kept them grouped together separately to have better readability instead of bloating the Indexes Endpoint class.

Now it may be better to put them inside another sub directory Eg:src/Endpoints/Delegates/Indexes to indicate that these traits belongs to Indexes class. But I didn't do it since we only have one such big Endpoint class, All other endpoints are contained in themselves.

[curquiza]

Hello @ppshobi!
Your PR is finally the next one to be merged! 🙏
Can you rebase please? 😁

[ppshobi]

Rebasing done. @curquiza can you add a rule to phpcs for using camelCase variables? since all our variables are camelCased and let's prevent new PR's from having snake_cased variables.

[curquiza]

Ok, I thought the naming convention for PHP variables was snake_case, but apparently it's not, it depends on what you decided at the beginning 🙂
@ppshobi Can you create an issue to suggest this change?
Thanks for your rebase, I'm doing the last check and then, I will merge.