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

⬆️ Laravel 12 compatibility #152

Merged
merged 9 commits into from
Mar 8, 2025
Merged

⬆️ Laravel 12 compatibility #152

merged 9 commits into from
Mar 8, 2025

Conversation

GautierDele
Copy link
Member

@GautierDele GautierDele commented Mar 8, 2025

closes #149, closes #114

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced search functionality now supports configurable result limits, allowing responses to reflect a user-specified cap or fall back to a default count.
    • Improved handling of associated data enables users to retrieve related records with a defined limit, offering more precise control over the displayed results.
    • Introduced predefined pagination limit values, including a new limit of 150, for better user experience in API responses.
  • Bug Fixes

    • Streamlined logic for handling search parameters, improving overall performance and clarity.
  • Tests

    • Added new tests to verify the functionality of search operations with limits and the inclusion of related records.
    • Expanded test coverage for search operations to ensure correct pagination and limit handling in API responses.

Copy link
Contributor

coderabbitai bot commented Mar 8, 2025

Walkthrough

The pull request updates the CI/CD pipeline configuration and dependency constraints to support newer PHP and Laravel versions. It enhances search functionality with more flexible limit handling by modifying query builder classes and methods, refines how responses for model relations are generated, and adds new tests to verify the behavior of search operations and relationship limits. Additionally, a new method is introduced to provide predefined limit values for resource responses.

Changes

File(s) Change Summary
.github/workflows/tests.yml Updated matrix configuration: PHP versions changed to 8.2, 8.3, 8.4; Laravel versions changed to ^11.0 and ^12.0; removed explicit exclusion for PHP 8.1 with Laravel ^11.0.
composer.json Updated dependency constraints: PHP from ^8.0 to ^8.2, laravel/framework to ^11.0
src/Actions/DispatchAction.php In handleClassic, added disableDefaultLimit() call and modified the return statement to use getLimit() if set, otherwise falling back to count.
src/Http/Response.php Simplified modelToResponse by removing logic for handling relation limits and pivot checks; now directly processes model relations.
src/Query/Builder.php Added a new disableDefaultLimit property and method to control query result limits.
src/Query/ScoutBuilder.php Updated search method to enforce a limit on results, defaulting to 50 if not specified.
src/Query/Traits/PerformSearch.php Simplified logic for handling sorts and restructured limit application in search method.
tests/Feature/Controllers/ActionsOperationsTest.php, tests/Feature/Controllers/SearchIncludingRelationshipsOperationsTest.php, tests/Support/Rest/Resources/BelongsToManyResource.php Introduced new tests assessing search functionality with limits and belongs-to-many relationships, and added a limits method in BelongsToManyResource for predefined pagination values.
README.md Updated system requirements from "PHP 8.1+ and Laravel 10.0+" to "PHP 8.2+ and Laravel 11+".
tests/Support/Rest/Resources/ModelResource.php Added a limit value of 150 to the limits method, expanding the range of acceptable limit values.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API
    participant DispatchAction
    participant QueryBuilder
    participant Database

    Client->>API: POST request with search parameters & limit
    API->>DispatchAction: Invoke handleClassic(searchQuery)
    DispatchAction->>QueryBuilder: Call disableDefaultLimit()
    QueryBuilder->>QueryBuilder: Apply limit (from parameters or default)
    QueryBuilder->>Database: Execute query with applied limit
    Database-->>QueryBuilder: Return result count or limited records
    QueryBuilder-->>DispatchAction: Return determined result count
    DispatchAction-->>API: Send final response
    API-->>Client: Return JSON with impacted record count
Loading

Poem

Oh, what a hop, what a bound,
The code has leaped without a sound.
New limits set and tests in sight,
Dependencies dance in pure delight.
With chains of logic so refined,
A bunny's cheer in every line!
🐰✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e665795 and 1b265df.

📒 Files selected for processing (1)
  • src/Actions/DispatchAction.php (3 hunks)
🔇 Additional comments (5)
src/Actions/DispatchAction.php (5)

112-123: Great documentation improvement.

The expanded method documentation clearly explains the purpose, functionality, and return values of the handleClassic method. This significantly improves code maintainability and makes it easier for other developers to understand what the method does.


131-135: Good implementation of Laravel 12 compatibility changes.

Disabling the default limit and capturing the explicit limit value from the base query is a necessary approach for Laravel 12 compatibility. This ensures the chunking logic will work correctly with the updated query builder behavior.


140-152: Verify $page indexing to prevent premature chunk termination.

The current chunking condition if (!is_null($limit) && $page * $chunkCount >= $limit) might terminate processing prematurely if $page starts at 1 and $chunkCount >= $limit. Consider using (($page - 1) * $chunkCount) + $chunkCount >= $limit which is equivalent but makes the logic clearer.

Let's verify how Laravel's chunk method handles page indexing in version 12:

#!/bin/bash
# Search for Laravel's chunk method implementation to confirm page indexing
grep -r "public function chunk" vendor/laravel/framework/src/Illuminate/Database/
# If not found, let's try a broader search
grep -r "function chunk" vendor/laravel/framework/src/Illuminate/Database/

156-156: Appropriate return value prioritization.

Returning $limit ?? $searchQuery->count() is a logical improvement that aligns with the method's documented behavior, prioritizing the limit when it exists and falling back to the total count otherwise.


224-224: Good type safety improvement.

Adding the return type declaration : self improves type safety and follows modern PHP best practices. This makes the code more robust and self-documenting.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Docstrings generation was requested by @GautierDele.

* #152 (comment)

The following files were modified:

* `src/Actions/DispatchAction.php`
* `src/Http/Response.php`
* `src/Query/Builder.php`
* `src/Query/ScoutBuilder.php`
* `src/Query/Traits/PerformSearch.php`
* `tests/Support/Rest/Resources/BelongsToManyResource.php`
Copy link
Contributor

coderabbitai bot commented Mar 8, 2025

Note

Generated docstrings for this pull request at #153

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
README.md (1)

9-9: Consider explicitly mentioning Laravel 12.

You've updated the system requirements to “PHP 8.2+ and Laravel 11+.” Given the PR objective of Laravel 12 compatibility, it may be clearer to say “Laravel 11+ (including 12)” or “Laravel 11|12” to avoid confusion and align with the updated pipeline matrix.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eeeba72 and e665795.

📒 Files selected for processing (5)
  • README.md (1 hunks)
  • composer.json (1 hunks)
  • src/Actions/DispatchAction.php (3 hunks)
  • tests/Feature/Controllers/ActionsOperationsTest.php (1 hunks)
  • tests/Support/Rest/Resources/ModelResource.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • composer.json
🧰 Additional context used
🪛 GitHub Actions: tests
tests/Feature/Controllers/ActionsOperationsTest.php

[error] 33-33: Failed asserting that 0 matches expected 2.


[error] 123-123: Failed asserting that 0 matches expected 150.


[error] 190-190: Failed asserting that 0 matches expected 2.


[error] 217-217: Failed asserting that 0 matches expected 150.


[error] 240-240: Failed asserting that 0 matches expected 2.


[error] 260-260: The expected [Lomkit\Rest\Actions\CallRestApiAction] job was not pushed. Failed asserting that false is true.


[error] 292-292: Failed asserting that 0 matches expected 1.


[error] 349-349: The expected batch was not dispatched. Failed asserting that false is true.

🔇 Additional comments (3)
tests/Support/Rest/Resources/ModelResource.php (1)

118-118: Nice addition of the 150-limit option.

This addition broadens supported limit values, which is especially helpful when working with large datasets. Be sure other related resource files or queries also account for an expanded limit if relevant.

tests/Feature/Controllers/ActionsOperationsTest.php (1)

298-333:

❓ Verification inconclusive

Test logic appears valid, but pipeline failures suggest the search or chunk logic may not be engaging properly.

The pipeline fails with discrepancies like “0 matches expected 150.” Possible reasons:

  1. The “string = match” filter might not be recognized or is being skipped by the chunk logic.
  2. The chunk callback might be terminating early if $page indexing is off.

Re-check the chunking and the “disableDefaultLimit” logic to ensure the 300 matching models are targeted and that the test environment is seeding data as expected.

To investigate, you could run this script to confirm seed data is properly saved:


🏁 Script executed:

#!/bin/bash
# Verify that 300 models with `string=match` were indeed created
# and the search parameter is recognized.

# 1. Grep logs or code for model creation.
rg -A 5 $'->count(300)->create([
    \'string\' => \'match\'
])'

# 2. Check if the test is referencing the correct environment or database
# (Adjust if a different DB connection is used)
# 3. Look for chunk usage or override in the code, ensuring chunk callback is invoked properly.
rg -A 10 'chunk('

Length of output: 561


I'll re-run the checks using corrected regex patterns to gather more reliable data. Please execute the following shell script to verify that:

#!/bin/bash
# 1. Confirm that the test file creates 300 models with 'string' set to 'match'
rg -n "->count(300)->create\(\[" tests/Feature/Controllers/ActionsOperationsTest.php

# 2. Locate any usage of the chunk callback in the repository with correct escaping for parentheses
rg -n "chunk\(" -A 10

Once you share the output of these commands, we can more confidently determine whether the test seed data and the chunking logic (including any "disableDefaultLimit" adjustments) are functioning as expected.

src/Actions/DispatchAction.php (1)

224-224: Good practice adding a return type.

Specifying : self clarifies the method’s fluent interface and can prevent certain type mismatches. Keep integrating return types throughout the code to maintain consistency.

@GautierDele GautierDele merged commit d6141f9 into master Mar 8, 2025
21 checks passed
@GautierDele GautierDele deleted the feature/laravel-12 branch March 8, 2025 19:42
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.

Package does not work on Laravel 12 eager load limit
2 participants