Skip to content

Conversation

@donatj
Copy link
Owner

@donatj donatj commented Nov 4, 2025

Adds next() to ResponseByMethod, which itself invokes next() on the most recently called method.

Fixes #81


Important

ResponseByMethod now implements MultiResponseInterface and adds next() to handle sub MultiResponseInterfaces, with tests verifying this behavior.

  • Behavior:
    • ResponseByMethod now implements MultiResponseInterface.
    • Adds next() method to ResponseByMethod to call next() on the most recently called method's response if it implements MultiResponseInterface.
  • Testing:
    • Adds ResponseByMethod_RegressionTest to verify next() behavior with ResponseStack for GET and DELETE methods.

This description was created by Ellipsis for a7989a5. You can customize this summary. It will automatically update as commits are pushed.

@donatj donatj force-pushed the fix/ResponseByMethod-call-next branch 2 times, most recently from 0214021 to 0395d14 Compare November 4, 2025 18:25
@donatj donatj requested a review from Copilot November 4, 2025 18:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for ResponseByMethod to implement MultiResponseInterface, enabling it to forward state management to method-specific responses that also implement this interface (like ResponseStack). This allows different HTTP methods to maintain independent response sequences when using stacked responses.

  • Converted ResponseByMethod from ResponseInterface to MultiResponseInterface
  • Added state tracking for the last HTTP method called and implemented the next() method
  • Added a regression test demonstrating the behavior with multiple methods using ResponseStack

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/ResponseByMethod.php Implements MultiResponseInterface and adds next() method to forward state management to method-specific responses
test/Regression/ResponseByMethod_RegressionTest.php Adds regression test verifying that ResponseByMethod correctly forwards next() calls to maintain independent state per HTTP method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Adds `next()` to ResponseByMethod which itself invokes next on the most recently called method.
@donatj donatj force-pushed the fix/ResponseByMethod-call-next branch from 0395d14 to a7989a5 Compare November 4, 2025 18:29
@donatj donatj marked this pull request as ready for review November 4, 2025 18:31
@donatj donatj merged commit 959257b into master Nov 4, 2025
42 checks passed
@donatj donatj deleted the fix/ResponseByMethod-call-next branch November 4, 2025 18:31
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to a7989a5 in 3 minutes and 7 seconds. Click for details.
  • Reviewed 120 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/ResponseByMethod.php:83
  • Draft comment:
    Delegating next() to the underlying MultiResponseInterface is correct. Consider documenting that when the fallback defaultResponse is used (i.e. when no response is set for the method), next() will always return false.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. src/ResponseByMethod.php:70
  • Draft comment:
    Caching the request method in 'latestMethod' for use in next() works well, but note that in concurrent scenarios, this mutable state may lead to race conditions. Documenting its intended usage in single-threaded/mock contexts might help.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. test/Regression/ResponseByMethod_RegressionTest.php:13
  • Draft comment:
    The regression test validates response sequencing via file_get_contents, which is good. Consider adding explicit tests that call next() on the ResponseByMethod instance to directly verify its delegation behavior.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_UcG49vOtdawekLCJ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

Nested ResponseByMethod does not work with ResponseStack in sequential requests

2 participants