Bump to PHP 8.1, improve type safety for Tasks #735
Conversation
norkunas
added
enhancement
norkunas
force-pushed
the
task-object
branch
3 times, most recently
from
00a7b98 to
31b5036
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
==========================================
- Coverage 89.78% 88.14% -1.65%
==========================================
Files 59 78 +19
Lines 1449 1687 +238
==========================================
+ Hits 1301 1487 +186
- Misses 148 200 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
YAY! If these are to be deprecated, are they going to be replaced with direct access to the object? public function getData(): array
{
return $this->data;
}
// @todo: deprecate
public function offsetExists(mixed $offset): bool
{
return \array_key_exists($offset, $this->data);
}
// @todo: deprecate
public function offsetGet(mixed $offset): mixed
{
return $this->data[$offset] ?? null;
}
// @todo: deprecate
public function offsetSet(mixed $offset, mixed $value): void
{
throw new \LogicException(\sprintf('Setting data on "%s::%s" is not supported.', get_debug_type($this), $offset));
}
// @todo: deprecate
public function offsetUnset(mixed $offset): void
{
throw new \LogicException(\sprintf('Unsetting data on "%s::%s" is not supported.', get_debug_type($this), $offset));
} |
|
@tacman if this PR will get merged, then you'll get access to both ways: |
|
That makes sense. I imagine there will be other things in 2.0 as well. |
|
Hi, thanks for your PR. Do we want to release this in a v2.x? Can you add a "migration guide" to your first comment to highlight the required implementation changes? |
Based on semver, it should be in v2.x, but if you want to be in line with meilisearch/meilisearch repository, I think it's reasonable to release this in v1.
Can you link to some previous comment where I could see how this looks like? |
|
Hi @norkunas, no need to follow a specific format. A simple before/after of the relevant parts of the code is enough. Here's an example for meilisearch JS: https://github.com/meilisearch/meilisearch-js/releases/tag/v0.48.0 It's mainly so I have examples to refer to when writing the release changelog. Double-checking with the actual code changes is better, but the code samples are a good resource too for me, especially if I write the changelog a long time after reviewing the PR. |
|
@Strift I've updated description, is it ok for you? |
|
I also have a side idea for another improvement: $task = $this->client->createIndex('new-index', ['primaryKey' => 'objectID']);
$task = $this->index->waitForTask($task->getTaskUid());we could write: $task = $this->client->createIndex('new-index', ['primaryKey' => 'objectID'])->wait();But not sure, as then we'd need for this method to work at least inject in in the Task object the |
|
Hello here! |
|
Hi, I'm for merging to v1 :) I think this is an acceptable BC break to provide type safety |
|
I also think that it would be good to have typed task |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the PHP version to 8.1 and refactors numerous endpoint methods to return the new Task object for improved type safety. Key changes include adding missing type hints with union types, converting array responses into Task objects via Task::fromArray, and updating the code style and CI workflows to support only PHP 8.1+.
Reviewed Changes
Copilot reviewed 66 out of 66 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Endpoints/Delegates/HandlesSystem.php | Updated swapIndexes method signature to return Task but without wrapping the response in a Task object. |
| src/Endpoints/Delegates/HandlesSnapshots.php | Changed return type to Task. |
| src/Endpoints/Delegates/HandlesSettings.php | Updated numerous settings methods to wrap responses with Task::fromArray. |
| src/Endpoints/Delegates/HandlesIndex.php | Adjusted index-related methods to return Task and improved type annotations. |
| src/Endpoints/Delegates/HandlesDumps.php | Updated createDump return type to Task. |
| src/Endpoints/Delegates/HandlesDocuments.php | Refactored document-related methods to return Task with appropriate wrapping. |
| src/Endpoints/Delegates/HandlesBatches.php | Refined the parameter type. |
| Other files (Contracts, Client, composer.json, etc.) | Updated types, enums, and CI configurations to support PHP 8.1+ and modern syntax. |
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
This mostly looks good to me. I left a few comments.
I would love to release this with the changes related to Meilisearch v1.15.
Also, Codecov says the following lines are not covered by tests:
- src/Contracts/Task.php#L114
- src/Contracts/Task.php#L116
|
Regarding the other topics of discussion in this thread:
This could be a welcome addition, but I prefer having it in a separate PR to keep this one's scope minimal. |
|
Thanks @Strift for review, I'm currently preparing for |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces array-based task responses with a typed Task model and TaskDetails, converts many endpoint mutators to return Task, tightens PHP types (requires ^8.1), adds Task::wait() integration and partial helper, enables JSON_THROW_ON_ERROR for serializers, and updates tests and CI matrices accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Endpoint
participant Http
participant TaskFactory as Task::fromArray
participant Poll as Tasks::waitTask
Note over TaskFactory,Poll: partial binds Poll to Http
Caller->>Endpoint: call mutatingOperation(...)
Endpoint->>Http: POST/PUT → raw response (array)
Http-->>Endpoint: response array
Endpoint->>TaskFactory: Task::fromArray(response, boundPoll)
TaskFactory-->>Endpoint: Task
Endpoint-->>Caller: Task
alt Caller invokes wait()
Caller->>TaskFactory: task->wait()
loop poll until finished
Poll->>Http: GET /tasks/{uid}
Http-->>Poll: latest task data
Poll->>TaskFactory: Task::fromArray(...)
TaskFactory-->>Poll: Task
end
Poll-->>Caller: final finished Task
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)src/Contracts/TaskDetails/UnknownTaskDetails.php (1)
src/Contracts/Task.php (5)
🔇 Additional comments (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
norkunas
force-pushed
the
task-object
branch
from
28c45ee to
db46570
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Contracts/SimilarDocumentsQuery.php (1)
144-148: Align type signatures: both methods should accept?floatfor consistency.The type discrepancy is confirmed and intentional—SimilarDocumentsQuery's test explicitly includes
@testWith [123]and[0.123], showing int values are tested. However, since both methods represent the same semantic concept (ranking score threshold), they should use the same type. A ranking score threshold is semantically a float value; accepting int adds unnecessary flexibility. ChangeSimilarDocumentsQuery::setRankingScoreThreshold()to accept?float(matching SearchQuery), and update the property type on line 45 fromprivate int|float|nulltoprivate ?float. Update the test data provider to use only float values.tests/Endpoints/DocumentsTest.php (2)
631-632: Migrate to Task-based API for consistency.This test still uses the deprecated array-based pattern (
$promise = ...addDocuments(...); waitForTask($promise['taskUid'])), while the rest of the file has been migrated to the new Task API with->wait()chaining.Apply this diff to align with the new pattern:
- $promise = $index->addDocuments(self::DOCUMENTS); - $index->waitForTask($promise['taskUid']); + $index->addDocuments(self::DOCUMENTS)->wait();
646-647: Migrate to Task-based API for consistency.Another instance of the deprecated array-based pattern. This should be updated to match the Task-based approach used elsewhere in the file.
Apply this diff:
- $promise = $index->addDocuments(self::DOCUMENTS); - $index->waitForTask($promise['taskUid']); + $index->addDocuments(self::DOCUMENTS)->wait();tests/Endpoints/SimilarDocumentsTest.php (1)
20-20: Add->wait()to ensure documents are indexed before tests run.The
updateDocuments()call on line 20 is missing->wait(), while every other test in the suite that callsupdateDocuments()chains.wait()on it. Since embedders are prerequisites that must be configured before document vectors can be indexed, and the test immediately performs a search after updating settings, the document indexing task could still be in progress when the search executes—potentially causing flaky test failures.This is particularly evident in
SearchTest.phpline 695, which explicitly waits for both the embedder configuration and the document indexing before proceeding.- $this->index->updateDocuments(self::VECTOR_MOVIES); + $this->index->updateDocuments(self::VECTOR_MOVIES)->wait();
♻️ Duplicate comments (2)
src/Contracts/TaskDetails/DocumentEditionDetails.php (1)
32-41: Add null coalescing operators for all fields to prevent undefined index warnings.All fields are accessed directly without guards, which can cause undefined index warnings if Meilisearch omits optional fields. For consistency with
DocumentDeletionDetailsand other TaskDetails classes, use the null coalescing operator:public static function fromArray(array $data): self { return new self( - $data['context'], - $data['deletedDocuments'], - $data['editedDocuments'], - $data['function'], - $data['originalFilter'], + $data['context'] ?? [], + $data['deletedDocuments'] ?? null, + $data['editedDocuments'] ?? null, + $data['function'] ?? null, + $data['originalFilter'] ?? null, ); }Note: A past review comment indicated this was addressed in commit a1e9277, but the current code doesn't reflect these guards. Please verify whether changes were inadvertently reverted.
tests/Contracts/TaskTest.php (1)
26-28: Date format inconsistencies reduce test clarity.The constructor parameters (lines 26-28) use varied date formats (space vs T separator, with/without timezone suffix, nanosecond vs microsecond precision), while assertions (lines 45-47) expect different formats. Although tests pass due to timestamp normalization, consistent formats improve maintainability.
As suggested in past reviews, standardize on ISO 8601 format with Z timezone:
enqueuedAt: new \DateTimeImmutable('2025-04-09 10:28:12.236789123Z'), - startedAt: new \DateTimeImmutable('2025-04-09 10:28:12.555566'), - finishedAt: new \DateTimeImmutable('2025-04-09 10:28:12.666677'), + startedAt: new \DateTimeImmutable('2025-04-09T10:28:12.555566Z'), + finishedAt: new \DateTimeImmutable('2025-04-09T10:28:12.666677Z'),And update assertions to match:
- self::assertEquals(new \DateTimeImmutable('2025-04-09T10:28:12.236789'), $task->getEnqueuedAt()); - self::assertEquals(new \DateTimeImmutable('2025-04-09T10:28:12.555566+0000'), $task->getStartedAt()); - self::assertEquals(new \DateTimeImmutable('2025-04-09T10:28:12.666677+0000'), $task->getFinishedAt()); + self::assertEquals(new \DateTimeImmutable('2025-04-09T10:28:12.236789Z'), $task->getEnqueuedAt()); + self::assertEquals(new \DateTimeImmutable('2025-04-09T10:28:12.555566Z'), $task->getStartedAt()); + self::assertEquals(new \DateTimeImmutable('2025-04-09T10:28:12.666677Z'), $task->getFinishedAt());Also applies to: 45-47
🧹 Nitpick comments (2)
src/Contracts/TaskDetails/IndexSwapDetails.php (1)
24-29: Consider defensive key access in fromArray.The
fromArraymethod directly accesses$data['swaps']without checking if the key exists. WhileTask::fromArrayonly calls this when details are present, defensive coding would make this more robust.Apply this diff for safer access:
public static function fromArray(array $data): self { return new self( - $data['swaps'], + $data['swaps'] ?? [], ); }src/Endpoints/Tasks.php (1)
25-29: Address TODO: wrap task array results in Task objects.The
all()method currently returns a raw array, but the TODO comment indicates it should returnarray<Task>. Each task in the results should be wrapped withTask::fromArray()for consistency with other methods.Would you like me to generate the implementation that maps each result to a Task object?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (89)
.php-cs-fixer.dist.php(1 hunks)composer.json(2 hunks)phpunit.xml.dist(2 hunks)src/Client.php(1 hunks)src/Contracts/Data.php(2 hunks)src/Contracts/Http.php(1 hunks)src/Contracts/SimilarDocumentsQuery.php(4 hunks)src/Contracts/Task.php(1 hunks)src/Contracts/TaskDetails.php(1 hunks)src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php(1 hunks)src/Contracts/TaskDetails/DocumentDeletionDetails.php(1 hunks)src/Contracts/TaskDetails/DocumentEditionDetails.php(1 hunks)src/Contracts/TaskDetails/DumpCreationDetails.php(1 hunks)src/Contracts/TaskDetails/IndexCreationDetails.php(1 hunks)src/Contracts/TaskDetails/IndexDeletionDetails.php(1 hunks)src/Contracts/TaskDetails/IndexSwapDetails.php(1 hunks)src/Contracts/TaskDetails/IndexUpdateDetails.php(1 hunks)src/Contracts/TaskDetails/SettingsUpdateDetails.php(1 hunks)src/Contracts/TaskDetails/TaskCancelationDetails.php(1 hunks)src/Contracts/TaskDetails/TaskDeletionDetails.php(1 hunks)src/Contracts/TaskError.php(1 hunks)src/Contracts/TaskStatus.php(1 hunks)src/Contracts/TaskType.php(1 hunks)src/Contracts/TasksResults.php(2 hunks)src/Endpoints/Batches.php(1 hunks)src/Endpoints/Delegates/HandlesBatches.php(1 hunks)src/Endpoints/Delegates/HandlesDocuments.php(8 hunks)src/Endpoints/Delegates/HandlesDumps.php(1 hunks)src/Endpoints/Delegates/HandlesIndex.php(2 hunks)src/Endpoints/Delegates/HandlesSettings.php(21 hunks)src/Endpoints/Delegates/HandlesSnapshots.php(1 hunks)src/Endpoints/Delegates/HandlesSystem.php(2 hunks)src/Endpoints/Delegates/HandlesTasks.php(2 hunks)src/Endpoints/Delegates/TasksQueryTrait.php(1 hunks)src/Endpoints/Dumps.php(1 hunks)src/Endpoints/Indexes.php(6 hunks)src/Endpoints/Keys.php(4 hunks)src/Endpoints/Snapshots.php(1 hunks)src/Endpoints/Tasks.php(1 hunks)src/Endpoints/TenantToken.php(2 hunks)src/Exceptions/ApiException.php(3 hunks)src/Exceptions/CommunicationException.php(1 hunks)src/Exceptions/InvalidArgumentException.php(0 hunks)src/Exceptions/InvalidResponseBodyException.php(2 hunks)src/Exceptions/LogicException.php(1 hunks)src/Exceptions/TimeOutException.php(1 hunks)src/Http/Client.php(3 hunks)src/Http/Serialize/Json.php(1 hunks)src/Http/Serialize/SerializerInterface.php(2 hunks)src/Search/FacetSearchResult.php(1 hunks)src/Search/SearchResult.php(1 hunks)src/functions.php(1 hunks)tests/Contracts/TaskTest.php(1 hunks)tests/Endpoints/BatchesTest.php(1 hunks)tests/Endpoints/ClientTest.php(3 hunks)tests/Endpoints/DocumentsTest.php(38 hunks)tests/Endpoints/DumpTest.php(1 hunks)tests/Endpoints/FacetSearchTest.php(1 hunks)tests/Endpoints/IndexTest.php(5 hunks)tests/Endpoints/MultiSearchTest.php(1 hunks)tests/Endpoints/SearchNestedFieldsTest.php(4 hunks)tests/Endpoints/SearchTest.php(24 hunks)tests/Endpoints/SimilarDocumentsTest.php(1 hunks)tests/Endpoints/SnapshotsTest.php(1 hunks)tests/Endpoints/TasksTest.php(4 hunks)tests/Endpoints/TenantTokenTest.php(3 hunks)tests/MockTask.php(1 hunks)tests/Settings/DisplayedAttributesTest.php(2 hunks)tests/Settings/DistinctAttributeTest.php(1 hunks)tests/Settings/EmbeddersTest.php(5 hunks)tests/Settings/FacetSearchTest.php(1 hunks)tests/Settings/FacetingAttributesTest.php(1 hunks)tests/Settings/FilterableAttributesTest.php(3 hunks)tests/Settings/LocalizedAttributesTest.php(2 hunks)tests/Settings/NonSeparatorTokensTest.php(1 hunks)tests/Settings/PaginationTest.php(1 hunks)tests/Settings/PrefixSearchTest.php(1 hunks)tests/Settings/ProximityPrecisionTest.php(2 hunks)tests/Settings/RankingRulesTest.php(2 hunks)tests/Settings/SearchCutoffMsTest.php(2 hunks)tests/Settings/SearchableAttributesTest.php(1 hunks)tests/Settings/SeparatorTokensTest.php(1 hunks)tests/Settings/SettingsTest.php(3 hunks)tests/Settings/SortableAttributesTest.php(1 hunks)tests/Settings/StopWordsTest.php(1 hunks)tests/Settings/SynonymsTest.php(1 hunks)tests/Settings/TypoToleranceTest.php(2 hunks)tests/Settings/WordDictionaryTest.php(2 hunks)tests/TestCase.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/Exceptions/InvalidArgumentException.php
🚧 Files skipped from review as they are similar to previous changes (42)
- src/Search/FacetSearchResult.php
- tests/Settings/SynonymsTest.php
- tests/Endpoints/BatchesTest.php
- src/Contracts/TaskStatus.php
- src/Contracts/TasksResults.php
- src/Exceptions/InvalidResponseBodyException.php
- src/Contracts/TaskDetails/IndexCreationDetails.php
- src/Contracts/TaskDetails.php
- tests/Settings/SearchableAttributesTest.php
- tests/Settings/DistinctAttributeTest.php
- tests/TestCase.php
- src/Contracts/TaskError.php
- tests/Settings/SettingsTest.php
- src/Http/Serialize/Json.php
- src/Endpoints/Dumps.php
- src/Contracts/TaskDetails/SettingsUpdateDetails.php
- src/Contracts/TaskDetails/TaskDeletionDetails.php
- tests/Settings/TypoToleranceTest.php
- tests/Settings/EmbeddersTest.php
- src/Exceptions/TimeOutException.php
- src/Endpoints/Delegates/TasksQueryTrait.php
- src/Endpoints/Delegates/HandlesSnapshots.php
- tests/Settings/ProximityPrecisionTest.php
- src/Exceptions/ApiException.php
- tests/Settings/NonSeparatorTokensTest.php
- tests/Settings/SearchCutoffMsTest.php
- tests/Settings/RankingRulesTest.php
- src/Endpoints/Delegates/HandlesSystem.php
- tests/Settings/WordDictionaryTest.php
- src/Contracts/TaskDetails/IndexUpdateDetails.php
- src/Contracts/TaskDetails/DumpCreationDetails.php
- src/Search/SearchResult.php
- tests/Endpoints/SnapshotsTest.php
- tests/Settings/DisplayedAttributesTest.php
- src/Endpoints/Delegates/HandlesDumps.php
- src/Contracts/TaskDetails/DocumentDeletionDetails.php
- src/Endpoints/Indexes.php
- src/Contracts/Data.php
- tests/Endpoints/TenantTokenTest.php
- src/Endpoints/Keys.php
- src/Http/Client.php
- src/Endpoints/Batches.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T05:05:22.383Z
Learnt from: bpolaszek
Repo: meilisearch/meilisearch-php PR: 779
File: tests/Endpoints/DocumentsTest.php:669-682
Timestamp: 2025-08-20T05:05:22.383Z
Learning: In Meilisearch, tasks have implicit dependencies where document operations (like addDocuments) won't be processed until prerequisite setting tasks (like updateSortableAttributes, updateFilterableAttributes) have completed. Therefore, waiting for the addDocuments task completion implicitly ensures that all prerequisite setting tasks have also completed.
Applied to files:
tests/Endpoints/SimilarDocumentsTest.phptests/Endpoints/DocumentsTest.phptests/Endpoints/MultiSearchTest.phptests/Endpoints/SearchTest.phpsrc/Endpoints/Delegates/HandlesDocuments.php
🧬 Code graph analysis (39)
src/functions.php (1)
src/Meilisearch.php (1)
Meilisearch(7-18)
tests/Settings/SeparatorTokensTest.php (2)
src/Endpoints/Delegates/HandlesSettings.php (3)
updateSeparatorTokens(352-355)getSeparatorTokens(344-347)resetSeparatorTokens(357-360)src/Contracts/Task.php (1)
wait(119-130)
tests/MockTask.php (2)
src/Contracts/Task.php (1)
Task(20-185)src/Endpoints/Indexes.php (1)
create(78-83)
tests/Endpoints/SimilarDocumentsTest.php (2)
src/Endpoints/Indexes.php (1)
updateSettings(253-256)src/Contracts/Task.php (1)
wait(119-130)
tests/Settings/FacetSearchTest.php (3)
src/Endpoints/Delegates/HandlesSettings.php (3)
updateFacetSearch(470-473)getFacetSearch(462-465)resetFacetSearch(478-481)src/Contracts/Task.php (1)
wait(119-130)tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)
src/Endpoints/Snapshots.php (4)
src/Contracts/Task.php (2)
Task(20-185)fromArray(150-184)src/functions.php (1)
partial(18-23)src/Contracts/Endpoint.php (1)
Endpoint(7-23)src/Endpoints/Tasks.php (2)
Tasks(16-67)waitTask(50-66)
src/Exceptions/CommunicationException.php (3)
src/Exceptions/ApiException.php (1)
__toString(32-53)src/Exceptions/InvalidResponseBodyException.php (1)
__toString(24-33)src/Exceptions/TimeOutException.php (1)
__toString(20-28)
src/Contracts/TaskDetails/IndexSwapDetails.php (2)
src/Contracts/Task.php (1)
fromArray(150-184)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
src/Contracts/TaskDetails/DocumentEditionDetails.php (4)
src/Contracts/Task.php (2)
__construct(28-43)fromArray(150-184)src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (2)
__construct(21-25)fromArray(27-33)src/Contracts/TaskDetails/DocumentDeletionDetails.php (2)
__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
src/Contracts/TaskDetails/TaskCancelationDetails.php (4)
src/Contracts/Task.php (2)
__construct(28-43)fromArray(150-184)src/Contracts/TaskDetails/DocumentDeletionDetails.php (2)
__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails/TaskDeletionDetails.php (2)
__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
src/Http/Serialize/SerializerInterface.php (1)
src/Http/Serialize/Json.php (2)
serialize(9-12)unserialize(14-17)
tests/Endpoints/DocumentsTest.php (6)
src/Contracts/Task.php (2)
Task(20-185)wait(119-130)src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (1)
DocumentAdditionOrUpdateDetails(15-34)src/Endpoints/Indexes.php (2)
Indexes(27-282)create(78-83)tests/MockTask.php (2)
MockTask(11-23)create(13-22)tests/TestCase.php (3)
TestCase(16-171)createEmptyIndex(131-136)safeIndexName(138-141)src/Endpoints/Delegates/HandlesDocuments.php (16)
addDocuments(46-49)getDocuments(28-44)addDocumentsInBatches(69-78)addDocumentsCsv(56-59)addDocumentsJson(51-54)addDocumentsNdjson(61-64)getDocument(21-26)updateDocuments(108-111)updateDocumentsInBatches(131-140)updateDocumentsByFunction(180-183)addDocumentsCsvInBatches(83-92)addDocumentsNdjsonInBatches(97-106)deleteDocument(190-193)deleteDocuments(195-208)deleteAllDocuments(185-188)updateDocumentsJson(113-116)
tests/Endpoints/MultiSearchTest.php (3)
tests/TestCase.php (1)
createEmptyIndex(131-136)src/Endpoints/Delegates/HandlesDocuments.php (2)
updateDocuments(108-111)addDocumentsCsv(56-59)src/Contracts/Task.php (1)
wait(119-130)
tests/Settings/SortableAttributesTest.php (3)
src/Endpoints/Delegates/HandlesSettings.php (3)
getSortableAttributes(265-268)updateSortableAttributes(273-276)resetSortableAttributes(278-281)tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)src/Contracts/Task.php (1)
wait(119-130)
tests/Settings/PaginationTest.php (3)
tests/TestCase.php (3)
TestCase(16-171)createEmptyIndex(131-136)safeIndexName(138-141)src/Endpoints/Delegates/HandlesSettings.php (3)
getPagination(161-164)updatePagination(169-172)resetPagination(174-177)src/Contracts/Task.php (1)
wait(119-130)
tests/Settings/LocalizedAttributesTest.php (3)
src/Endpoints/Delegates/HandlesIndex.php (1)
index(32-35)src/Endpoints/Delegates/HandlesSettings.php (2)
updateLocalizedAttributes(122-125)resetLocalizedAttributes(127-130)src/Contracts/Task.php (1)
wait(119-130)
tests/Endpoints/FacetSearchTest.php (2)
src/Endpoints/Delegates/HandlesSettings.php (1)
updateFilterableAttributes(250-253)src/Contracts/Task.php (1)
wait(119-130)
tests/Settings/FacetingAttributesTest.php (3)
src/Endpoints/Delegates/HandlesSettings.php (3)
getFaceting(137-141)updateFaceting(146-149)resetFaceting(151-154)tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)src/Contracts/Task.php (1)
wait(119-130)
src/Contracts/Http.php (1)
src/Http/Client.php (3)
post(87-98)put(108-119)patch(121-129)
src/Contracts/TaskDetails/IndexDeletionDetails.php (3)
src/Contracts/Task.php (2)
__construct(28-43)fromArray(150-184)src/Contracts/TaskDetails/DocumentDeletionDetails.php (2)
__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
src/Endpoints/Delegates/HandlesIndex.php (2)
src/Contracts/Task.php (1)
Task(20-185)src/Endpoints/Indexes.php (5)
fetchRawInfo(130-133)Indexes(27-282)fetchInfo(135-140)delete(147-153)create(78-83)
tests/Endpoints/IndexTest.php (8)
src/Contracts/Task.php (9)
Task(20-185)wait(119-130)getIndexUid(56-59)getStatus(61-64)getType(66-69)getDuration(89-92)getStartedAt(76-79)getFinishedAt(81-84)getDetails(104-107)src/Contracts/TaskDetails/IndexSwapDetails.php (1)
IndexSwapDetails(14-30)src/Contracts/TaskDetails/TaskDeletionDetails.php (1)
TaskDeletionDetails(16-38)src/Contracts/TasksQuery.php (1)
TasksQuery(9-96)src/Endpoints/Indexes.php (4)
update(142-145)getTasks(170-183)delete(147-153)swapIndexes(158-161)src/Contracts/TasksResults.php (1)
getResults(42-45)src/Endpoints/Delegates/HandlesSystem.php (1)
swapIndexes(51-56)src/Endpoints/Tasks.php (1)
deleteTasks(38-43)
tests/Endpoints/TasksTest.php (11)
src/Contracts/Task.php (7)
Task(20-185)getTaskUid(48-51)getType(66-69)getIndexUid(56-59)getDetails(104-107)getStatus(61-64)wait(119-130)src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (1)
DocumentAdditionOrUpdateDetails(15-34)src/Contracts/TaskDetails/TaskCancelationDetails.php (1)
TaskCancelationDetails(16-38)src/Contracts/TasksQuery.php (4)
TasksQuery(9-96)setLimit(54-59)setBatchUid(64-69)setReverse(71-76)src/Endpoints/Indexes.php (2)
getTask(165-168)getTasks(170-183)src/Endpoints/Delegates/HandlesTasks.php (3)
getTask(18-21)getTasks(23-30)cancelTasks(37-40)src/Contracts/TasksResults.php (2)
getResults(42-45)getTotal(74-77)tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)src/Endpoints/Delegates/HandlesDocuments.php (1)
updateDocuments(108-111)src/Endpoints/Tasks.php (1)
cancelTasks(31-36)src/Endpoints/Delegates/TasksQueryTrait.php (2)
setAfterEnqueuedAt(78-83)setUids(58-63)
tests/Endpoints/SearchNestedFieldsTest.php (5)
tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)src/Endpoints/Delegates/HandlesDocuments.php (1)
updateDocuments(108-111)src/Contracts/Task.php (1)
wait(119-130)src/Endpoints/Delegates/HandlesSettings.php (2)
updateSearchableAttributes(76-79)updateSortableAttributes(273-276)src/Endpoints/Indexes.php (1)
updateSettings(253-256)
src/Endpoints/Tasks.php (5)
src/Contracts/Task.php (3)
Task(20-185)fromArray(150-184)isFinished(114-117)src/functions.php (1)
partial(18-23)src/Http/Client.php (3)
get(69-77)post(87-98)delete(131-139)src/Contracts/Http.php (3)
get(16-16)post(24-24)delete(44-44)src/Endpoints/Indexes.php (2)
all(85-98)delete(147-153)
tests/Settings/PrefixSearchTest.php (3)
src/Endpoints/Delegates/HandlesSettings.php (3)
updatePrefixSearch(500-503)getPrefixSearch(490-493)resetPrefixSearch(508-511)src/Contracts/Task.php (1)
wait(119-130)tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)
src/Endpoints/Delegates/HandlesTasks.php (3)
src/Contracts/Task.php (1)
Task(20-185)src/Contracts/TasksResults.php (1)
TasksResults(7-94)src/Endpoints/Tasks.php (3)
Tasks(16-67)deleteTasks(38-43)cancelTasks(31-36)
src/Contracts/SimilarDocumentsQuery.php (2)
src/Search/SimilarDocumentsSearchResult.php (1)
__construct(29-38)src/Contracts/SearchQuery.php (1)
setRankingScoreThreshold(275-280)
tests/Endpoints/SearchTest.php (5)
tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)src/Endpoints/Delegates/HandlesDocuments.php (2)
updateDocuments(108-111)addDocuments(46-49)src/Contracts/Task.php (1)
wait(119-130)src/Endpoints/Indexes.php (1)
delete(147-153)src/Endpoints/Delegates/HandlesSettings.php (6)
updateSearchableAttributes(76-79)updateFilterableAttributes(250-253)updateRankingRules(30-33)updateSortableAttributes(273-276)updateEmbedders(444-447)updateLocalizedAttributes(122-125)
tests/Settings/StopWordsTest.php (2)
src/Endpoints/Delegates/HandlesSettings.php (3)
updateStopWords(192-195)getStopWords(184-187)resetStopWords(197-200)src/Contracts/Task.php (1)
wait(119-130)
tests/Endpoints/DumpTest.php (2)
src/Endpoints/Delegates/HandlesDumps.php (1)
createDump(14-17)src/Contracts/Task.php (1)
getType(66-69)
src/Endpoints/TenantToken.php (2)
src/Exceptions/InvalidArgumentException.php (2)
InvalidArgumentException(7-26)emptyArgument(9-16)src/Endpoints/Delegates/HandlesSystem.php (1)
generateTenantToken(46-49)
tests/Endpoints/ClientTest.php (3)
tests/TestCase.php (1)
safeIndexName(138-141)src/Endpoints/Delegates/HandlesIndex.php (5)
createIndex(56-59)updateIndex(64-67)index(32-35)getIndex(40-43)deleteIndex(48-51)src/Contracts/Task.php (2)
wait(119-130)getIndexUid(56-59)
src/Contracts/Task.php (14)
src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (3)
DocumentAdditionOrUpdateDetails(15-34)__construct(21-25)fromArray(27-33)src/Contracts/TaskDetails/DocumentDeletionDetails.php (3)
DocumentDeletionDetails(16-38)__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails/DocumentEditionDetails.php (3)
DocumentEditionDetails(18-42)__construct(23-30)fromArray(32-41)src/Contracts/TaskDetails/DumpCreationDetails.php (3)
DumpCreationDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexCreationDetails.php (3)
IndexCreationDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexDeletionDetails.php (3)
IndexDeletionDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexSwapDetails.php (3)
IndexSwapDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexUpdateDetails.php (3)
IndexUpdateDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/SettingsUpdateDetails.php (3)
SettingsUpdateDetails(55-148)__construct(99-121)fromArray(123-147)src/Contracts/TaskDetails/TaskCancelationDetails.php (3)
TaskCancelationDetails(16-38)__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails/TaskDeletionDetails.php (3)
TaskDeletionDetails(16-38)__construct(23-28)fromArray(30-37)src/Exceptions/LogicException.php (1)
LogicException(7-9)src/Contracts/TaskError.php (3)
__construct(15-21)TaskError(7-40)fromArray(31-39)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (2)
src/Contracts/Task.php (1)
fromArray(150-184)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
tests/Contracts/TaskTest.php (8)
src/Contracts/Task.php (14)
Task(20-185)getTaskUid(48-51)getIndexUid(56-59)getStatus(61-64)getType(66-69)getEnqueuedAt(71-74)getStartedAt(76-79)getFinishedAt(81-84)getDuration(89-92)getCanceledBy(94-97)getBatchUid(99-102)getDetails(104-107)getError(109-112)wait(119-130)src/Contracts/TaskDetails/IndexCreationDetails.php (1)
IndexCreationDetails(14-30)src/Contracts/TaskError.php (1)
TaskError(7-40)src/Exceptions/LogicException.php (1)
LogicException(7-9)tests/MockTask.php (2)
MockTask(11-23)create(13-22)src/Endpoints/Indexes.php (1)
create(78-83)src/Endpoints/Dumps.php (1)
create(16-19)src/Endpoints/Snapshots.php (1)
create(16-19)
tests/Settings/FilterableAttributesTest.php (3)
src/Endpoints/Delegates/HandlesSettings.php (3)
getFilterableAttributes(237-240)updateFilterableAttributes(250-253)resetFilterableAttributes(255-258)tests/TestCase.php (2)
createEmptyIndex(131-136)safeIndexName(138-141)src/Contracts/Task.php (1)
wait(119-130)
src/Endpoints/Delegates/HandlesDocuments.php (6)
src/Contracts/Task.php (2)
Task(20-185)fromArray(150-184)src/Endpoints/Tasks.php (2)
Tasks(16-67)waitTask(50-66)src/functions.php (1)
partial(18-23)src/Http/Client.php (3)
post(87-98)put(108-119)delete(131-139)src/Contracts/Http.php (3)
post(24-24)put(32-32)delete(44-44)src/Endpoints/Indexes.php (1)
delete(147-153)
src/Endpoints/Delegates/HandlesSettings.php (6)
src/Contracts/Task.php (2)
Task(20-185)fromArray(150-184)src/Endpoints/Tasks.php (3)
Tasks(16-67)waitTask(50-66)get(20-23)src/functions.php (1)
partial(18-23)src/Http/Client.php (4)
put(108-119)delete(131-139)patch(121-129)get(69-77)src/Contracts/Http.php (4)
put(32-32)delete(44-44)patch(38-38)get(16-16)src/Endpoints/Indexes.php (1)
delete(147-153)
🪛 GitHub Actions: Tests
tests/Endpoints/IndexTest.php
[error] 251-251: PHP-CS-Fixer formatting check failed due to indentation issue. Run 'vendor/bin/php-cs-fixer fix' to apply fixes.
🪛 GitHub Check: phpstan-tests
tests/Endpoints/IndexTest.php
[failure] 253-253:
Undefined variable: $response
[failure] 252-252:
Undefined variable: $response
[failure] 251-251:
Undefined variable: $response
[failure] 250-250:
Undefined variable: $response
🪛 PHPMD (2.15.0)
tests/Endpoints/IndexTest.php
248-248: Avoid unused local variables such as '$task'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (40)
src/Exceptions/CommunicationException.php (1)
9-12: LGTM! Type declaration aligns with PHP 8.1 best practices.The explicit
: stringreturn type improves type safety and is consistent with other exception classes in the codebase (ApiException, InvalidResponseBodyException, TimeOutException).src/Endpoints/Delegates/HandlesBatches.php (1)
15-15: LGTM! Parameter type addition improves type safety.The addition of the
inttype declaration for the$uidparameter aligns with the PR's goal of improving type safety and is consistent with the underlyingBatches::get(int $batchUid)method signature.phpunit.xml.dist (1)
2-37: LGTM! Enhanced test strictness aligns with v2 type safety goals.The PHPUnit configuration improvements include:
- Display attributes (lines 10-14) that surface detailed information about deprecations, errors, notices, and warnings during test runs
- Schema location (line 4) changed to use the local vendor path instead of a remote URL, ensuring consistency with the installed PHPUnit version
- Suppression-ignoring attributes (lines 26-32) that report issues even when suppressed with the
@operatorThese stricter settings are appropriate for a v2 release focused on type safety and will help catch potential issues early.
src/Http/Serialize/SerializerInterface.php (1)
16-16: LGTM! Type safety improvement for serialize.The explicit
mixed $dataparameter and: stringreturn type improve type safety and align well with the PR objective. The docblock's more specific union type provides valuable additional information for static analyzers.src/Contracts/SimilarDocumentsQuery.php (2)
45-45: Good improvement: explicit default and type declaration.Adding the explicit
= nulldefault andint|float|nulltype declaration improves clarity and type safety. This aligns well with the PR's goal of stronger typing.
51-55: Type safety improvements look good.Adding explicit type declarations to the constructor parameters aligns well with the PR's PHP 8.1 upgrade and type safety objectives. The constructor now enforces types at runtime, improving robustness.
.php-cs-fixer.dist.php (1)
18-19: LGTM! Configuration changes align with PR objectives.The
php_unit_strictrule is appropriately disabled to prevent test breakage from automaticassertEqualstoassertSameconversions. The addition of'parameters'totrailing_comma_in_multilineis consistent with modern PHP 8.1+ coding standards and improves code maintainability.tests/Settings/FacetingAttributesTest.php (1)
20-48: LGTM! Clean migration to Task-based API.The refactoring successfully migrates from promise-based task handling to the new fluent Task API with inline
->wait()chaining. This eliminates intermediate variables and improves readability while maintaining the same test coverage.tests/Settings/LocalizedAttributesTest.php (1)
28-45: LGTM! Consistent Task-based refactoring.The test methods properly utilize the new Task API with inline
->wait()calls, eliminating the need for intermediate promise storage and separate task waiting logic.tests/Settings/StopWordsTest.php (1)
30-41: LGTM! Clean Task-based implementation.The migration to the Task API is well-executed, with direct
->wait()chaining that simplifies the test logic and improves code clarity.tests/Settings/SortableAttributesTest.php (1)
15-37: LGTM! Successful Task-based migration.All test methods correctly implement the new fluent Task API, using
->wait()for synchronous task completion and direct assertions on the resulting state.src/Client.php (1)
56-56: LGTM! Trailing comma improves maintainability.Adding a trailing comma after the last parameter is consistent with modern PHP 8.1+ best practices and the
trailing_comma_in_multilinerule enabled in this PR. This makes future parameter additions cleaner in version control diffs.composer.json (4)
34-34: LGTM! PHP 8.1 requirement aligns with PR objectives.The PHP version bump to 8.1 is intentional and documented in the PR objectives. This enables the use of modern PHP features like mixed types, readonly properties, and other enhancements that support the Task-based API improvements.
44-47: LGTM! Files autoload enables the partial() helper.Adding
src/functions.phpto the autoload files is necessary for the newpartial()helper function used throughout the codebase for binding wait callbacks in Task-based operations.
60-60: LGTM! PHPUnit 10.5 aligns with PHP 8.1 requirement.Consolidating to PHPUnit 10.5 only is appropriate given the PHP 8.1 minimum requirement, as PHPUnit 10 requires PHP 8.1+.
36-36: I'll search for more specific information about the php-http/discovery package and its compatibility.No incompatibility issues found with php-http/discovery 1.19.
The package requires PHP ^7.1 || ^8.0, which includes PHP 8.1. Recent releases include fixes for compatibility with PHP 8.4 and Symfony 7, indicating active maintenance. The version increment from 1.7 to 1.19 remains within the stable 1.x series with no breaking changes documented that would impact this upgrade.
tests/Endpoints/DumpTest.php (1)
7-16: LGTM! Clean migration to Task-based assertion.The test correctly migrates from array key validation to type-safe Task object assertions using the TaskType enum. This aligns well with the PR's goal of replacing array returns with strongly-typed objects.
tests/Endpoints/FacetSearchTest.php (1)
21-21: LGTM! Cleaner fluent wait pattern.The test now uses a direct chained wait instead of the two-step promise pattern, making the code more concise and leveraging the new Task-based API effectively.
src/Endpoints/Delegates/HandlesTasks.php (2)
18-21: Breaking change: Task-based return type improves type safety.The method signature now enforces
int $uidand returns a strongly-typedTaskobject instead of an array, which is a breaking change but aligns with the PR's core objective. This provides better developer experience with IDE autocomplete and static analysis.
32-40: Breaking change: Task-based returns and wait methods moved to Task class.The
deleteTasksandcancelTasksmethods now returnTaskobjects instead of arrays. The previouswaitForTask()andwaitForTasks()methods have been removed in favor of calling->wait()directly on the returned Task object, enabling fluent chaining like$client->deleteTasks(...)->wait().src/Exceptions/LogicException.php (1)
1-9: LGTM! Standard domain-specific exception.This follows the established pattern of wrapping PHP's built-in exceptions with library-specific implementations that implement
ExceptionInterface. Used appropriately inTask::wait()when the wait function is not provided.tests/Settings/FacetSearchTest.php (2)
20-27: LGTM! Simplified test with fluent wait chaining.The test now uses the new Task-based wait pattern, eliminating intermediate promise variables and making the test flow more readable.
29-37: LGTM! Clean sequential task pattern.The sequential wait pattern (
update->wait(); reset->wait()) clearly expresses the test flow and ensures operations complete before assertions.src/functions.php (1)
7-23: LGTM! Well-documented partial application helper.The
partial()function provides a clean way to bind arguments (like the HTTP client) to create reusable wait callbacks. The implementation is correct, and the documentation clearly explains its purpose and usage. The@internalannotation appropriately indicates this is for internal SDK use.tests/Settings/PrefixSearchTest.php (2)
20-26: LGTM! Clean Task-based wait pattern.The test correctly uses the fluent
->wait()chaining introduced by the Task object, making the test more concise and readable compared to the previous promise-based approach.
28-36: LGTM! Sequential waits properly ensure operation completion.The sequential pattern with explicit waits ensures each operation completes before the next one begins, which is correct for testing reset functionality.
tests/Endpoints/MultiSearchTest.php (1)
29-29: LGTM! Task-based fluent wait pattern correctly applied.The changes successfully migrate from the old promise-based pattern to the new Task-based fluent API. Using
->wait()inline simplifies the test setup and aligns with the PR's objective to introduce typed Task objects.Also applies to: 37-37
src/Endpoints/Snapshots.php (1)
16-18: LGTM! Correctly migrated to Task-based return.The endpoint properly wraps the HTTP response with
Task::fromArrayand binds the wait callback usingpartial(Tasks::waitTask(...), $this->http), consistent with the pattern used across other endpoints in this PR.tests/Settings/SeparatorTokensTest.php (1)
37-39: LGTM! Tests correctly updated to Task-based pattern.The tests now use the fluent
->wait()API instead of capturing task UIDs and waiting separately. This simplifies the test flow and is consistent with the Task-based approach adopted throughout the PR.Also applies to: 44-47
tests/Settings/PaginationTest.php (1)
32-37: LGTM! Pagination tests follow the Task-based pattern correctly.The tests properly use the fluent
->wait()API for asynchronous operations and assert results directly viagetPagination(). The implementation is consistent with other settings tests in the suite.Also applies to: 39-45
tests/MockTask.php (1)
13-22: LGTM! Useful test helper with sensible defaults.The
MockTaskhelper provides a convenient way to create Task instances for testing with reasonable defaults. The use of named parameters for theawaitclosure is correct and aligns with the Task constructor signature.tests/Settings/FilterableAttributesTest.php (1)
23-25: LGTM! FilterableAttributes tests properly migrated to Task pattern.All test methods correctly use the fluent
->wait()API and assert results directly. The migration is consistent with the Task-based approach used throughout the test suite.Also applies to: 33-36, 57-59, 66-83
src/Contracts/TaskType.php (1)
7-21: LGTM! Clean enum definition.The TaskType enum is well-structured with comprehensive coverage of Meilisearch task types. The string-backed values follow consistent camelCase naming.
src/Endpoints/Delegates/HandlesIndex.php (1)
9-67: LGTM! Task-based return types enable fluent API.The updated method signatures returning
Taskinstead ofarrayalign perfectly with the PR's Task-based refactoring. The added PHPDocnon-empty-stringannotations improve type safety for static analysis.tests/Endpoints/ClientTest.php (1)
91-141: LGTM! Tests correctly demonstrate the new Task-based API.The test updates showcase the fluent chaining pattern (
->wait()) and typed accessors (->getIndexUid()). The method rename (line 91) also fixes the capitalization inconsistency.src/Endpoints/TenantToken.php (1)
21-32: LGTM! Empty object validation fixed.The cast to array on line 26 correctly addresses the past review concern. Both empty arrays and empty objects are now properly rejected before they can produce invalid tenant tokens.
tests/Contracts/TaskTest.php (1)
84-114: LGTM! Comprehensive Task behavior coverage.The wait mechanism tests properly exercise both async completion (lines 84-94) and immediate return for finished tasks (lines 96-104). The exception test (lines 106-114) correctly validates the error case when no await function is provided.
tests/Endpoints/DocumentsTest.php (1)
75-79: LGTM! Excellent demonstration of Task-based assertions.This segment showcases the benefits of the new API: typed status checks via
TaskStatusenum and structured details access throughDocumentAdditionOrUpdateDetails. This pattern should be used throughout the test suite.tests/Endpoints/SearchNestedFieldsTest.php (1)
19-111: LGTM! Clean migration and typo fixes.The file demonstrates consistent adoption of the Task-based API with
->wait()chaining (lines 19, 70, 88, 108-111). The method name corrections (lines 68, 86, 106) fix "Atributes" → "Attributes" typos, improving code quality.tests/Endpoints/SimilarDocumentsTest.php (1)
25-25: LGTM! Correctly adopts the new Task-based API.The change properly uses the new fluent chaining pattern with
->wait()on the Task object returned byupdateSettings(), ensuring the embedders configuration is applied before the similar documents search is performed.
| public static function fromArray(array $data): self | ||
| { | ||
| return new self( | ||
| $data['deletedDocuments'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add null coalescing operator for nullable field.
The deletedDocuments property is nullable, and the docblock indicates it's null while the task is enqueued or processing. However, the fromArray method doesn't guard against a missing array key, which could trigger an undefined index warning.
For consistency with DocumentDeletionDetails (shown in relevant snippets), use the null coalescing operator:
public static function fromArray(array $data): self
{
return new self(
- $data['deletedDocuments'],
+ $data['deletedDocuments'] ?? null,
);
}🤖 Prompt for AI Agents
In src/Contracts/TaskDetails/IndexDeletionDetails.php around lines 24 to 29, the
fromArray method accesses $data['deletedDocuments'] directly which can raise an
undefined index warning for the nullable deletedDocuments field; change it to
use the null coalescing operator to default to null (e.g.
$data['deletedDocuments'] ?? null) so the constructor receives null when the key
is absent.
| public static function fromArray(array $data): self | ||
| { | ||
| return new self( | ||
| $data['matchedTasks'], | ||
| $data['canceledTasks'], | ||
| $data['originalFilter'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify array key existence to prevent undefined index warnings.
The fromArray method directly accesses array keys without checking their existence. If the Meilisearch API response omits these keys in certain scenarios, this will trigger undefined array key warnings in PHP 8.
Note that DocumentDeletionDetails uses the null coalescing operator ($data['providedIds'] ?? null) while TaskDeletionDetails uses direct access like this implementation. For consistency and defensive coding, consider using the null coalescing operator.
Apply this diff if keys might be absent:
public static function fromArray(array $data): self
{
return new self(
- $data['matchedTasks'],
- $data['canceledTasks'],
- $data['originalFilter'],
+ $data['matchedTasks'] ?? null,
+ $data['canceledTasks'] ?? null,
+ $data['originalFilter'] ?? null,
);
}Run the following script to verify consistency across TaskDetails implementations and check whether the API guarantees these keys:
🏁 Script executed:
#!/bin/bash
# Check how other TaskDetails implementations handle array access
echo "=== Checking fromArray patterns across TaskDetails implementations ==="
rg -A 10 'public static function fromArray\(array \$data\): self' --type php -g 'src/Contracts/TaskDetails/*.php'
echo ""
echo "=== Looking for usages with potentially missing keys ==="
rg 'TaskCancelationDetails::fromArray' --type php -B 3 -A 3Length of output: 8156
I need to gather more information about how the API responses are tested and whether keys can be missing.
Let me search for test files and more specific information about task cancelation.
The evidence shows critical findings. Let me check the constructor of TaskCancelationDetails to understand what types it accepts:
Let me search the codebase directly for the TaskCancelationDetails class definition:
You have used 50,500 tokens so far (25% of your available budget). You have 149,500 tokens remaining.Based on my verification, I found real API responses where canceledTasks can be null while a task is processing. Combined with the codebase evidence showing inconsistent patterns across TaskDetails implementations where some use defensive null coalescing and others don't, the review comment's concern is valid.
The issue is that TaskCancelationDetails and TaskDeletionDetails use direct array access without null coalescing, while DocumentDeletionDetails and SettingsUpdateDetails use the null coalescing operator.
Use the null coalescing operator on all three array keys in TaskCancelationDetails::fromArray().
Apply the suggested diff to add defensive null coalescing to matchedTasks, canceledTasks, and originalFilter keys, ensuring consistency with DocumentDeletionDetails and handling cases where the API response omits these keys or returns null values.
public static function fromArray(array $data): self
{
return new self(
- $data['matchedTasks'],
- $data['canceledTasks'],
- $data['originalFilter'],
+ $data['matchedTasks'] ?? null,
+ $data['canceledTasks'] ?? null,
+ $data['originalFilter'] ?? null,
);
}🏁 Script executed:
#!/bin/bash
# Find TaskCancelationDetails class definition and constructor
echo "=== TaskCancelationDetails class definition ==="
cat src/Contracts/TaskDetails/TaskCancelationDetails.php
echo ""
echo "=== Check if constructor uses nullable types or has defaults ==="
rg -A 20 'class TaskCancelationDetails' src/Contracts/TaskDetails/TaskCancelationDetails.phpLength of output: 2364
Use the null coalescing operator on all three array keys in TaskCancelationDetails::fromArray().
The constructor already declares these parameters as nullable (?int and ?string), and the documentation explicitly states that canceledTasks will be null during enqueued or processing status. The fromArray() method must defensively handle cases where these keys are missing or omitted from the API response, which will cause undefined array key errors in PHP 8.1+.
public static function fromArray(array $data): self
{
return new self(
- $data['matchedTasks'],
- $data['canceledTasks'],
- $data['originalFilter'],
+ $data['matchedTasks'] ?? null,
+ $data['canceledTasks'] ?? null,
+ $data['originalFilter'] ?? null,
);
}🤖 Prompt for AI Agents
In src/Contracts/TaskDetails/TaskCancelationDetails.php around lines 30 to 37,
fromArray() directly indexes $data['matchedTasks'], $data['canceledTasks'], and
$data['originalFilter'] which can throw "undefined index" errors; change each to
use the null coalescing operator (e.g. $data['matchedTasks'] ?? null,
$data['canceledTasks'] ?? null, $data['originalFilter'] ?? null) so the nullable
constructor parameters receive null when keys are missing.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/meilisearch-beta-tests.yml(1 hunks).github/workflows/pre-release-tests.yml(1 hunks).github/workflows/tests.yml(1 hunks)src/Http/Serialize/Json.php(1 hunks)src/Http/Serialize/SerializerInterface.php(2 hunks)tests/Endpoints/IndexTest.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Http/Serialize/SerializerInterface.php
- src/Http/Serialize/Json.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Endpoints/IndexTest.php (7)
src/Contracts/Task.php (9)
Task(20-185)wait(119-130)getIndexUid(56-59)getStatus(61-64)getType(66-69)getDuration(89-92)getStartedAt(76-79)getFinishedAt(81-84)getDetails(104-107)src/Contracts/TaskDetails/IndexSwapDetails.php (1)
IndexSwapDetails(14-30)src/Contracts/TaskDetails/TaskDeletionDetails.php (1)
TaskDeletionDetails(16-38)src/Contracts/TasksQuery.php (1)
TasksQuery(9-96)src/Endpoints/Delegates/HandlesIndex.php (3)
index(32-35)getIndex(40-43)createIndex(56-59)src/Endpoints/Indexes.php (4)
update(142-145)getTasks(170-183)delete(147-153)swapIndexes(158-161)src/Endpoints/Delegates/HandlesDocuments.php (1)
addDocuments(46-49)
🪛 GitHub Check: phpstan-tests
tests/Endpoints/IndexTest.php
[failure] 254-254:
Offset 'rename' does not exist on array{indexes: array{'indexC', 'indexD'}}.
[failure] 253-253:
Offset 'rename' does not exist on array{indexes: array{'indexA', 'indexB'}}.
🔇 Additional comments (10)
.github/workflows/tests.yml (1)
80-80: PHP test matrix properly narrowed to align with 8.1+ requirement.The version matrix now tests against PHP 8.1, 8.2, 8.3, and 8.4, correctly reflecting the minimum version bump. This is consistent with the PR objective and aligns with the PHP ^8.1 requirement in composer.json.
Note: The v1.x branch (line 9) will also use this newer matrix. Verify this is intentional or consider branch-specific matrix configuration if v1.x should maintain legacy PHP support separately.
.github/workflows/pre-release-tests.yml (1)
43-43: Pre-release test matrix correctly updated to PHP 8.1–8.4.Consistent with the version narrowing in the main tests workflow and aligned with the PHP ^8.1 requirement.
.github/workflows/meilisearch-beta-tests.yml (1)
43-43: Beta test matrix correctly updated to PHP 8.1–8.4.Consistent with the version narrowing across all three test workflows, properly reflecting the PHP ^8.1 requirement.
tests/Endpoints/IndexTest.php (7)
8-13: LGTM!The new imports align perfectly with the Task-based API migration and are used consistently throughout the test file.
118-129: LGTM!The refactoring correctly uses the Task API with
wait()chaining and typedgetIndexUid()accessor.
173-186: LGTM!The test correctly uses
Task->getIndexUid()in the array mapping and properly chainswait()calls.
188-219: LGTM!The wait tests thoroughly cover different timeout/interval combinations and correctly assert on TaskStatus/TaskType enums and nullable timestamp fields.
221-228: LGTM!The test correctly validates timeout handling by calling
wait(0, 20)to force an immediate timeout and expectsTimeOutException.
230-244: LGTM!The test correctly validates the immediate Task response from
delete()without waiting for completion, which is appropriate for verifying task metadata.
257-263: LGTM!The test correctly validates task deletion by accessing typed details through
TaskDeletionDetailsand itsoriginalFilterproperty.
|
@Strift enums basically are already value objects, so I don't see a point in wrapping them. |
|
Btw |
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
Great work! All things LGTM now.
I have a question regarding the parsing of task types/statuses. Should we add a PSR-3 warning?
e.g. "Unknown Meilisearch task status: <status>; mapped to TaskStatus::Unknown. Consider upgrading the SDK for typed support."
Well for now this library did not depend on the PSR-3, but sure we can adopt it if you want :) This can be for follow-up PR, with a new issue opened |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Contracts/TaskDetails/UnknownTaskDetails.php (1)
9-20: Consider clarifying PHPDoc type parameter.The PHPDoc declares
@implements TaskDetails<array{}>(empty array shape) but the constructor acceptsarray<mixed>and can store arbitrary data. Since UnknownTaskDetails is designed to handle any unknown task detail structure, the PHPDoc type parameter should likely bearray<mixed>orarray<array-key, mixed>for accuracy.Apply this diff if you want the PHPDoc to match the actual behavior:
/** - * @implements TaskDetails<array{}> + * @implements TaskDetails<array<mixed>> */ final class UnknownTaskDetails implements TaskDetails
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Contracts/Http.php(2 hunks)src/Contracts/Task.php(1 hunks)src/Contracts/TaskDetails/IndexSwapDetails.php(1 hunks)src/Contracts/TaskDetails/UnknownTaskDetails.php(1 hunks)src/Contracts/TaskStatus.php(1 hunks)src/Contracts/TaskType.php(1 hunks)src/Endpoints/Delegates/HandlesSettings.php(21 hunks)src/Http/Client.php(7 hunks)tests/Endpoints/DocumentsTest.php(40 hunks)tests/Endpoints/MultiModalSearchTest.php(1 hunks)tests/Settings/ChatTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Contracts/TaskDetails/IndexSwapDetails.php
- src/Contracts/Http.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T05:05:22.383Z
Learnt from: bpolaszek
Repo: meilisearch/meilisearch-php PR: 779
File: tests/Endpoints/DocumentsTest.php:669-682
Timestamp: 2025-08-20T05:05:22.383Z
Learning: In Meilisearch, tasks have implicit dependencies where document operations (like addDocuments) won't be processed until prerequisite setting tasks (like updateSortableAttributes, updateFilterableAttributes) have completed. Therefore, waiting for the addDocuments task completion implicitly ensures that all prerequisite setting tasks have also completed.
Applied to files:
tests/Endpoints/DocumentsTest.php
🧬 Code graph analysis (7)
tests/Endpoints/MultiModalSearchTest.php (2)
src/Contracts/Task.php (1)
wait(120-131)src/Endpoints/Delegates/HandlesDocuments.php (1)
addDocuments(46-49)
tests/Settings/ChatTest.php (2)
src/Endpoints/Delegates/HandlesSettings.php (1)
updateChat(596-599)src/Contracts/Task.php (1)
wait(120-131)
src/Contracts/TaskDetails/UnknownTaskDetails.php (1)
src/Contracts/Task.php (2)
__construct(29-44)fromArray(151-187)
tests/Endpoints/DocumentsTest.php (5)
src/Contracts/Task.php (4)
Task(21-188)wait(120-131)getStatus(62-65)getDetails(105-108)src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (1)
DocumentAdditionOrUpdateDetails(15-34)tests/MockTask.php (2)
MockTask(11-23)create(13-22)src/Endpoints/Delegates/HandlesDocuments.php (18)
addDocuments(46-49)addDocumentsInBatches(69-78)addDocumentsCsv(56-59)addDocumentsJson(51-54)addDocumentsNdjson(61-64)getDocument(21-26)updateDocuments(108-111)updateDocumentsInBatches(131-140)addDocumentsCsvInBatches(83-92)addDocumentsNdjsonInBatches(97-106)deleteDocument(190-193)deleteDocuments(195-208)deleteAllDocuments(185-188)updateDocumentsJson(113-116)updateDocumentsCsv(118-121)updateDocumentsNdjson(123-126)updateDocumentsCsvInBatches(145-154)updateDocumentsNdjsonInBatches(159-168)src/Endpoints/Delegates/HandlesSettings.php (1)
updateEmbedders(444-447)
src/Http/Client.php (1)
src/Contracts/Http.php (4)
post(24-24)put(32-32)patch(38-38)postStream(50-50)
src/Contracts/Task.php (15)
src/Contracts/TaskDetails/DocumentAdditionOrUpdateDetails.php (3)
DocumentAdditionOrUpdateDetails(15-34)__construct(21-25)fromArray(27-33)src/Contracts/TaskDetails/DocumentDeletionDetails.php (3)
DocumentDeletionDetails(16-38)__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails/DocumentEditionDetails.php (3)
DocumentEditionDetails(18-42)__construct(23-30)fromArray(32-41)src/Contracts/TaskDetails/DumpCreationDetails.php (3)
DumpCreationDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexCreationDetails.php (3)
IndexCreationDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexDeletionDetails.php (3)
IndexDeletionDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexSwapDetails.php (3)
IndexSwapDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/IndexUpdateDetails.php (3)
IndexUpdateDetails(14-30)__construct(19-22)fromArray(24-29)src/Contracts/TaskDetails/SettingsUpdateDetails.php (3)
SettingsUpdateDetails(55-148)__construct(99-121)fromArray(123-147)src/Contracts/TaskDetails/TaskCancelationDetails.php (3)
TaskCancelationDetails(16-38)__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails/TaskDeletionDetails.php (3)
TaskDeletionDetails(16-38)__construct(23-28)fromArray(30-37)src/Contracts/TaskDetails/UnknownTaskDetails.php (3)
UnknownTaskDetails(12-26)__construct(17-20)fromArray(22-25)src/Exceptions/LogicException.php (1)
LogicException(7-9)src/Contracts/TaskError.php (3)
__construct(15-21)TaskError(7-40)fromArray(31-39)src/Contracts/TaskDetails.php (1)
fromArray(15-15)
src/Endpoints/Delegates/HandlesSettings.php (6)
src/Contracts/Task.php (2)
Task(21-188)fromArray(151-187)src/Endpoints/Tasks.php (3)
Tasks(16-67)waitTask(50-66)get(20-23)src/functions.php (1)
partial(18-23)src/Http/Client.php (4)
put(108-119)delete(131-139)patch(121-129)get(69-77)src/Contracts/Http.php (4)
put(32-32)delete(44-44)patch(38-38)get(16-16)src/Endpoints/Indexes.php (1)
delete(147-153)
🔇 Additional comments (24)
src/Http/Client.php (4)
43-50: LGTM: Trailing comma improves maintainability.The trailing comma in the constructor parameter list aligns with the enabled
trailing_comma_in_multilinecoding style rule and makes future parameter additions cleaner in diffs.
87-87: LGTM: Explicitmixedtype aligns with interface contract.The explicit
mixedtype declaration for the$bodyparameter acrosspost(),put(),patch(), andpostStream()methods correctly aligns with the updated interface signatures insrc/Contracts/Http.php. The implementations properly handle serialization and document\JsonExceptionin their throws clauses.Also applies to: 108-108, 121-121, 147-147
216-228: LGTM: Explicit empty-array check improves URL formatting.The explicit
[] !== $queryParamsguard prevents appending a trailing?when no query parameters are present, resulting in cleaner URLs. This is a functional improvement over relying on implicit behavior.
261-264: LGTM: Cleaner implementation with modern PHP syntax.The refactored
isJSONResponse()usingarray_filter()with a static arrow function andstr_contains()is more readable and idiomatic for PHP 8.1+. Thestringtype hint on the parameter correctly matches PSR-7'sgetHeader()return type.src/Contracts/TaskType.php (1)
7-22: LGTM! Well-structured enum with forward compatibility.The string-backed enum correctly defines all known Meilisearch task types with the Unknown case providing graceful fallback for future task types. The DocumentEdition case has been validated against Meilisearch documentation per past discussions.
src/Contracts/TaskStatus.php (1)
14-14: LGTM! Unknown case enables forward compatibility.The addition of the Unknown status case follows the same graceful fallback pattern as TaskType, allowing the SDK to handle new task statuses introduced in future Meilisearch versions without breaking.
tests/Endpoints/MultiModalSearchTest.php (2)
29-34: LGTM! Clean migration to fluent Task-based waiting.The change from promise-based handling to fluent
->wait()chaining simplifies the test setup while maintaining the same synchronous behavior.
39-39: LGTM! Consistent with fluent wait pattern.The addDocuments call now uses the same fluent
->wait()pattern as updateSettings above, creating a consistent and readable test setup.tests/Settings/ChatTest.php (1)
57-57: LGTM! Consistent fluent wait pattern.The migration from promise-based handling to fluent
->wait()simplifies the test code while maintaining identical behavior.tests/Endpoints/DocumentsTest.php (6)
9-18: LGTM! Import changes align with Task-based refactor.The imports correctly add the new Task-related types and remove the now-unused InvalidArgumentException, reflecting the shift to Task-based async handling throughout the test suite.
26-26: LGTM! Clean fluent wait pattern.The direct
->wait()call eliminates the unused variable noted in past reviews while maintaining the same synchronous test behavior.
35-41: LGTM! Correct batch Task handling.The code properly captures the Task array returned by
addDocumentsInBatchesand waits for each task in a loop, correctly handling the batch operation semantics.
75-79: LGTM! Proper Task status and details validation.The test correctly validates the Task status and asserts on the typed DocumentAdditionOrUpdateDetails, demonstrating proper use of the new Task object model.
369-384: LGTM! Correct MockTask usage in test expectations.The test properly uses
MockTask::create(TaskType::DocumentEdition)to create stub Task objects in the mocked method callback, correctly modeling the new Task-based return type.
896-896: LGTM! Improved type signature.The explicit
?arrayreturn type improves type safety and makes the nullable return clearer compared to the previous implicit mixed return type.src/Contracts/Task.php (5)
29-44: LGTM! Well-typed immutable constructor.The constructor properly defines all Task properties as readonly with appropriate types, including the optional
$awaitclosure for wait functionality. The PHPDoc accurately describes the closure signature.
49-113: LGTM! Complete getter coverage with correct types.All readonly properties have corresponding getter methods with accurate return types and PHPDoc annotations, providing a clean public API for Task inspection.
115-118: LGTM! Correct finished state logic.The
isFinished()method correctly identifies a task as finished when it's neither Enqueued nor Processing, matching Meilisearch task lifecycle semantics.
120-131: LGTM! Well-implemented wait method with proper error handling.The wait method correctly short-circuits for already-finished tasks, invokes the await closure when provided, and throws a clear LogicException when wait functionality is unavailable. The default timeout and interval values are reasonable.
151-187: LGTM! Comprehensive factory with proper type mapping.The
fromArrayfactory correctly:
- Handles legacy
uidfallback fortaskUid- Uses
?? nullfor optionalindexUidas resolved in past reviews- Gracefully handles unknown types/statuses with tryFrom and Unknown fallback
- Maps task types to appropriate TaskDetails implementations
- Includes explanatory comment for SnapshotCreation's null details
- Properly handles optional error and await closure
This implementation provides excellent forward compatibility with future Meilisearch versions.
src/Endpoints/Delegates/HandlesSettings.php (4)
10-13: LGTM! Correct imports for Task-based pattern.The imports properly add Task, Tasks endpoint, and the partial function needed for binding wait callbacks to all mutating operations.
30-37: LGTM! Consistent Task-based mutator pattern.Both
updateRankingRulesandresetRankingRulescorrectly return Task objects with the wait callback bound viapartial(Tasks::waitTask(...), $this->http), establishing the pattern followed throughout this file.
410-424: LGTM! Proper PHPDoc and Task return for searchCutoffMs.The PHPDoc correctly specifies
non-negative-intfor both getter and update method, andupdateSearchCutoffMsfollows the consistent Task-based pattern with proper wait callback binding.
596-599: LGTM! Chat settings correctly returns Task with wait support.The
updateChatmethod correctly returns a Task with the wait callback bound, matching the pattern used by all other mutating methods in this file. This resolves the issue noted in past reviews.
norkunas
force-pushed
the
task-object
branch
from
e7ec932 to
9275679
Compare
|
Thanks for this gigantic PR! I'm thrilled to merge it finally 🙌 congrats! |
|
Wahoo! Congrats and thanks. |
5 participants
Pull Request
Related issue
Fixes #703
Closes #739
What does this PR do?
trailing_comma_in_multilinecs fixer rule;php_unit_strictcs fixer rule, because it breaks tests;This introduces a BC break for someone who is extending meilisearch classes, but not for those who consumes the data, as Task object was added implementing\ArrayAccess, so all raw data can still be accessed.This introduces a BC break for task data access and for someone who is extending meilisearch classes.
Migration guide
Before
After
Before
After
Before
After
PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests