Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement attributesToRetrieve for getObjects method #273

Merged
merged 2 commits into from Feb 17, 2017

Conversation

JanPetr
Copy link
Contributor

@JanPetr JanPetr commented Feb 17, 2017

Fixes #258

*
* @return mixed
*
* @throws \Exception
*/
public function getObjects($objectIDs)
public function getObjects($objectIDs, $attributesToRetrieve = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should ask for an array of strings like we do for the other methods.
The documentation is bad for some of the other methods, we say we expect comma separated values where we are actually accepting arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to be consistent with getObject method where we take comma-separated string.
Definitely not something I'd approve and I'd definitely rather accept array, but for me consistency has higher priority right now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arg, I just tested and it doesn't work with an array in the 'getObject' method.
I would:

  • Make sure we support arrays in both getObject and getObjects
  • Change docbloc and documentation to array
  • Keep backward compat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, can you review @rayrutjes?

@@ -210,12 +214,14 @@ public function getObject($objectID, $attributesToRetrieve = null)
* Get several objects from this index.
*
* @param array $objectIDs the array of unique identifier of objects to retrieve
* @param array $attributesToRetrieve (optional) if set, contains the list of attributes to retrieve as a string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change the docblock here to 'array of strings' en not mention the 'separated by ,' anymore

@@ -224,6 +230,15 @@ public function getObjects($objectIDs)
$requests = array();
foreach ($objectIDs as $object) {
$req = array('indexName' => $this->indexName, 'objectID' => $object);

if ($attributesToRetrieve) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail if empty array is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct behavior. Empty array = all attributes are fetched

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not the current behaviour. Empty string returns only objectIDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the API behavior for getObject endpoint. For getObjects API returns all attributes on empty string/array.

@JanPetr JanPetr force-pushed the attributes_to_retrieve_for_getobjects branch from 4b4ccb6 to a4dd529 Compare February 17, 2017 16:56
@rayrutjes
Copy link
Member

LGTM!

@JanPetr JanPetr merged commit bc501ee into master Feb 17, 2017
@JanPetr JanPetr deleted the attributes_to_retrieve_for_getobjects branch February 17, 2017 19:17
shortcuts pushed a commit that referenced this pull request Apr 27, 2022
* chore(ci): skip spreading if generated/main does not exist

* chore: update implementation of gitBranchExists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants