Skip to content

Conversation

Strift
Copy link
Collaborator

@Strift Strift commented Jun 4, 2025

This is a draft PR to see CI results.

Related issue

See #749 for discussion regarding v2.0

Summary by CodeRabbit

  • Refactor

    • Replaced custom JSON encoding/decoding exceptions with the standard PHP \JsonException throughout the application.
    • Updated error messages related to JSON errors for clarity and consistency.
  • Tests

    • Adjusted tests to expect \JsonException and updated related error message assertions.

* Remove custom JSON exceptions

* Lint

* Update src/Http/Serialize/Json.php

Co-authored-by: Tomas Norkūnas <[email protected]>

---------

Co-authored-by: Tomas Norkūnas <[email protected]>
Copy link

coderabbitai bot commented Jun 4, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This change removes custom JSON exception classes and replaces their usage throughout the codebase with PHP's built-in \JsonException. All relevant docblocks, exception handling, and test expectations are updated accordingly. The custom exception classes are deleted, and their references are removed from imports and documentation.

Changes

Files/Groups Change Summary
src/Contracts/Http.php, src/Http/Serialize/SerializerInterface.php, src/Http/Client.php Updated docblocks to use @throws \JsonException instead of custom exceptions; removed related imports.
src/Exceptions/JsonEncodingException.php, src/Exceptions/JsonDecodingException.php Deleted custom exception classes.
src/Http/Serialize/Json.php Removed try-catch blocks and custom exception wrapping; now directly throws \JsonException.
tests/Endpoints/DocumentsTest.php, tests/Http/ClientTest.php, tests/Http/Serialize/JsonTest.php Updated tests to expect \JsonException and adjusted expected exception messages; removed custom exception imports.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Serializer (Json)
    participant PHP Engine

    Client->>Serializer (Json): serialize(data)
    Serializer (Json)->>PHP Engine: json_encode(data, JSON_THROW_ON_ERROR)
    alt On error
        PHP Engine-->>Client: throws \JsonException
    else Success
        PHP Engine-->>Serializer (Json): JSON string
        Serializer (Json)-->>Client: JSON string
    end
Loading

Possibly related PRs

  • meilisearch/meilisearch-php#759: The main PR and the retrieved PR both make identical changes to replace custom JSON exceptions with PHP's built-in \JsonException across the same files and methods, including updates to interface docblocks, exception class removals, and test adjustments.

Suggested labels

breaking-change

Suggested reviewers

  • brunoocasali
  • norkunas

Poem

A hop, a leap, a JSON cheer,
Custom exceptions disappear!
Now with PHP’s own in tow,
Errors are clearer—let them show!
Tests and docs all join the fun,
Simpler code for everyone.
🐇✨

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch v2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Strift Strift marked this pull request as draft June 4, 2025 04:27
@Strift Strift linked an issue Jun 4, 2025 that may be closed by this pull request
@Strift Strift added this to the Meilisearch PHP v2 milestone Jun 4, 2025
@Strift Strift added enhancement New feature or request breaking-change The related changes are breaking for the users labels Aug 22, 2025
@norkunas
Copy link
Collaborator

I don't understand this. Shouldn't there be a v1 branch and the main would be v2? Now I see v2 branch, and main as v1

@Strift
Copy link
Collaborator Author

Strift commented Sep 10, 2025

That's what we'll do.

I just created the v1.x branch that we'll use for the v1 maintenance.

Let's fix the conflicts here so we can merge this branch in main.

I will take care of updating the CI settings to ensure we can work as follow in the future:

  • Use main as v2 branch and publish beta releases on composer with 2.0.0-rc.X tags
  • Use v1.x as legacy branch

@Strift Strift mentioned this pull request Sep 12, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 2.0 mega issue
2 participants