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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
==========================================
- Coverage 90.74% 90.40% -0.34%
==========================================
Files 52 73 +21
Lines 1340 1595 +255
==========================================
+ Hits 1216 1442 +226
- Misses 124 153 +29 ☔ 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. |
| case IndexSwap = 'indexSwap'; | ||
| case DocumentAdditionOrUpdate = 'documentAdditionOrUpdate'; | ||
| case DocumentDeletion = 'documentDeletion'; | ||
| case DocumentEdition = 'documentEdition'; |
There was a problem hiding this comment.
Unless Meilisearch docs are outdated, documentEdition type does not exist: https://www.meilisearch.com/docs/reference/api/tasks#type
There was a problem hiding this comment.
Oh now I see that it was used in tests which broke after I removed it.
There was a problem hiding this comment.
@Strift as you can see meilisearch docs are outdated: https://github.com/meilisearch/meilisearch-php/actions/runs/15012425388/job/42183478341?pr=735
|
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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces strict type safety and replaces array return types for asynchronous task operations with a new immutable Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint
participant HTTP
participant Task
Client->>Endpoint: Perform async operation (e.g., addDocuments)
Endpoint->>HTTP: Send HTTP request
HTTP-->>Endpoint: Receive response (array)
Endpoint->>Task: Task::fromArray(response)
Endpoint-->>Client: Return Task object
Client->>Task: call wait()
Task->>HTTP: poll task status until finished or timeout
HTTP-->>Task: return updated task status
Task-->>Client: resolved Task object
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Rename `$response` to `$task` in tests
…able when deleting all documents)
|
@Strift can you upmerge to v2, because it requires changes here that are already done on v1.15 |
|
Hi @norkunas, yes! Thanks for updating the target branch to |
|
So, I think we need to make the tests pass and merge this branch :) Can you do this @norkunas then ask for my review again, please? |
|
@brunoocasali that's not up to me. v2 needs to be upmerged, because |
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
Taskobject with detailed status and type enums and multiple task detail classes for clearer task management.partialfunction for partial application of callables.Taskobject and pagination settings.Breaking Changes
Taskobjects instead of arrays for asynchronous operations (documents, indexes, settings, dumps, snapshots, tasks).Improvements
Taskobjects instead of raw arrays, improving clarity and type safety.Taskobjects.Bug Fixes
Tests
Taskobject usage.Taskobjects rather than array access.Chores