Fix getKeys method
#323
Fix getKeys method
#323
Conversation
alallema
changed the title
getKeys method
| $key = new self( | ||
| $this->http, | ||
| $attributes['key'], | ||
| $attributes['description'], | ||
| $attributes['actions'], | ||
| $attributes['indexes'], | ||
| ); | ||
| if ($attributes['expiresAt']) { | ||
| $key->expiresAt = date_create_from_format('Y-m-d\TH:i:s\Z', $attributes['expiresAt']); | ||
| } | ||
| if ($attributes['createdAt']) { | ||
| $key->createdAt = date_create_from_format('Y-m-d\TH:i:s.vu\Z', $attributes['createdAt']); | ||
| } | ||
| if ($attributes['updatedAt']) { | ||
| $key->updatedAt = date_create_from_format('Y-m-d\TH:i:s.vu\Z', $attributes['updatedAt']); | ||
| } |
There was a problem hiding this comment.
Since every parameter in __construct is optional, should expiresAt, createdAt and updatedAt not be passed to the constructor as well?
In this case we should maybe pass an option object instead of individual parameters
Should we move the if's in the __construct ?
There was a problem hiding this comment.
Yes, we can but it's not the behavior we want I think. The goal of not putting it in the __construct is that you can directly give it a DateTime instead of a string.
And this is a good question because we also can remove this if forest and put it in the function all() instead. But I think this newInstance method is more useful to translate a json document to a Key object.
What do you think?
There was a problem hiding this comment.
but we never use new Keys() we always use newInstance. Could you show me a use case where we lose in ease of use?
There was a problem hiding this comment.
Indeed, we never use new Key() in the class because this is not its purpose. But if you are a user and you want to create a Key you do like:
Key key = new Key();
key.setIndexes(new String[] {"*"});
key.setActions(new String[] {"*"});
key.setExpiresAt(null);
client.createKey(key);You can see more examples in the code-sample.yml file.
And for the newInstance examples I don't, I only used it here. But if you talk about fill() function (the old newInstance() function that I renamed) I used it a lot to fill the Key with a json response from Meilisearch.
Not sure is more clear?
There was a problem hiding this comment.
public function create(array $options = []): self
Is this expecting an instance of Keys or an option object?
There was a problem hiding this comment.
The both can work but the field expiredAt should be DateTime in all case.
There was a problem hiding this comment.
I understand my mistake! LGTM !
There was a problem hiding this comment.
Shouldn't DateTimeImmutable::createFromFormat() be used instead of date_create_from_format ? I mean it should be changed in property declaration and here, because what's the point to have a mutable VO?
alallema
requested review from
brunoocasali and
bidoubiwa
and removed request for
brunoocasali
| $key = new self( | ||
| $this->http, | ||
| $attributes['key'], | ||
| $attributes['description'], | ||
| $attributes['actions'], | ||
| $attributes['indexes'], | ||
| ); | ||
| if ($attributes['expiresAt']) { | ||
| $key->expiresAt = date_create_from_format('Y-m-d\TH:i:s\Z', $attributes['expiresAt']); | ||
| } | ||
| if ($attributes['createdAt']) { | ||
| $key->createdAt = date_create_from_format('Y-m-d\TH:i:s.vu\Z', $attributes['createdAt']); | ||
| } | ||
| if ($attributes['updatedAt']) { | ||
| $key->updatedAt = date_create_from_format('Y-m-d\TH:i:s.vu\Z', $attributes['updatedAt']); | ||
| } |
There was a problem hiding this comment.
I understand my mistake! LGTM !
|
bors merge |
324: Update version for the next release (v0.23.2) r=curquiza a=alallema This version makes this package compatible with MeiliSearch v0.27.0🎉 Check out the changelog of [MeiliSearch v0.27.0](https://github.com/meilisearch/MeiliSearch/releases/tag/v0.27.0) for more information about the changes. ## 🚀 Enhancements - Add new methods for the new typo tolerance settings #316 `@alallema` `index.getTypoTolerance()` `index.updateTypoTolerance(params)` `index.resetTypoTolerance()` - Ensure nested field support #317 `@alallema` - Add new search parameters highlightPreTag, highlightPostTag and cropMarker #318 `@alallema` ## 🐛 Bug Fixes * Fix `getKeys` method (#323) `@alallema` Co-authored-by: alallema <amelie@meilisearch.com>
The
Client::getKeysmethod returns the correct number of keys but the same object for all of them.This was due to the
newInstancemethod which returned the instance instead of a new Instance as its name indicated.