Add middlewares into prependToGroup - #74
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6ac06670
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| protected function updateMiddlewareGroupStatement(Closure $closure, int $groupIndex): void | ||
| { | ||
| $originalMiddlewareList = $closure->stmts[$groupIndex]->expr->args[1]->value->items; |
There was a problem hiding this comment.
If an existing bootstrap file uses the single-middleware form for this Laravel API, e.g. $middleware->prependToGroup('api', SomeMiddleware::class) or a string, this path dereferences ->items as if the second argument were always an array. Updating that group will then fail instead of normalizing the existing value and appending the new middleware.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds support in AppBootstrapBuilder for inserting/updating Laravel withMiddleware(...)->prependToGroup(...) statements in bootstrap/app.php, with fixtures/tests to validate output.
Changes:
- Introduces
AddMiddlewarePrependToGroupvisitor and exposes it viaAppBootstrapBuilder::addMiddlewarePrependToGroup(). - Adds PHPUnit coverage plus new fixtures/origin structures for prepend-to-group scenarios.
- Updates
docker-compose.ymlto maphost.docker.internaltohost-gateway.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/Visitors/AppBootstrapVisitors/AddMiddlewarePrependToGroup.php |
New AST visitor to add/merge prependToGroup() calls inside withMiddleware() closure. |
src/Builders/AppBootstrapBuilder.php |
Adds builder API addMiddlewarePrependToGroup() and import handling for class-based middleware. |
tests/AppBootstrapBuilderTest.php |
Adds tests covering new prepend-to-group behavior. |
tests/Support/Classes/FakeClass.php |
Test-only middleware class used in fixtures. |
tests/Support/OriginStructures/bootstrap_with_prepend_group.php |
New origin structure input for tests. |
tests/fixtures/AppBootstrapBuilderTest/bootstrap_with_prepend_group.php |
Expected output fixture for newly generated prepend-to-group calls. |
tests/fixtures/AppBootstrapBuilderTest/bootstrap_with_changed_prepend_group.php |
Expected output fixture when merging into existing prepend-to-group call. |
src/Visitors/AppBootstrapVisitors/AbstractAppBootstrapVisitor.php |
Minor change to typed class constant declaration. |
docker-compose.yml |
Adds extra_hosts mapping for host.docker.internal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 682a3ddf1b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { | ||
| $middlewareList = $this->getMiddlewareList(); | ||
|
|
||
| $methodCall = new MethodCall(new Variable('middleware'), new Identifier($this->targetMethod), [ |
There was a problem hiding this comment.
If the bootstrap file names the withMiddleware callback parameter anything other than $middleware, for example function (Middleware $m): void, this inserts $middleware->prependToGroup(...) and leaves the generated bootstrap code referencing an undefined variable. The visitor already has the closure in insertNode, so the new method call should be built with the actual first parameter name instead of a hard-coded one.
Useful? React with 👍 / 👎.
Coverage Report for CI Build 27944873134Coverage increased (+0.9%) to 99.549%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
AZabolotnikov
changed the title
AZabolotnikov
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e49fac41fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (class_exists($middleware)) { | ||
| $basename = class_basename($middleware); | ||
|
|
||
| $value = new ClassConstFetch(new Name($basename), 'class'); |
There was a problem hiding this comment.
When the bootstrap file already imports a different class with the same short name as the middleware being added, this drops the namespace and later adds another unaliased use, so the generated file either has a duplicate import name or FakeClass::class resolves to the wrong class. This can happen with common middleware basenames such as Authenticate; use a fully-qualified Name here or add aliasing before shortening the class name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f0e82bb7b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b80c3c90e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
artengin
left a comment
artengin
left a comment
There was a problem hiding this comment.
- Don't forget to add information about the new method to the
READMEdocumentation. - The task requirements mention a third argument,
position, but this has not been implemented yet. Please cover these cases with tests. - At the moment, creating the
withMiddlewaremethod when it is not present in the original file is not implemented. Please cover this case with tests.
@artengin @AZabolotnikov let's also implement as a separate PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c084939a1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| protected function findMiddlewareGroupIndex(array $stmts): ?int | ||
| { | ||
| return array_find_key($stmts, function (Expression $stmt) { |
There was a problem hiding this comment.
When a bootstrap already has more than one prependToGroup('api', ...) call, this returns only the first match, so mergeMiddlewares() checks duplicates against that first argument list only. If the middleware being added is already present in a later call for the same group, the visitor appends it to the first call as well and the generated bootstrap runs the same middleware twice; scan all matching calls, or consolidate them, before deciding the middleware is new.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 953dc6297e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
# Conflicts: # docker-compose.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbc208f5d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ->save(); | ||
| } | ||
|
|
||
| public function testAddMiddlewarePrependToGroupExistsMiddlewares() |
There was a problem hiding this comment.
This test isn't quite correct: it doesn't actually change the contents of prependToGroup, but it still adds an unnecessary import.
If the intent was to show that duplicate code isn't created inside prependToGroup, the fixture should have been named bootstrap_without_changed_prepend_group, and the import shouldn't appear either.
I suggest we either:
- In a follow-up PR, figure out how to avoid importing a class when no changes were made to
PrependToGroup(we'll need this capability in the future anyway), or - Simply rework the fixture and test to properly demonstrate that duplicates aren't created and the fixture remains unchanged.
Co-authored-by: Artyom Osepyan <152782500+artengin@users.noreply.github.com>
Co-authored-by: Artyom Osepyan <152782500+artengin@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4b39d031
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return $originalName === $newMiddleware | ||
| || $originalName === class_basename($newMiddleware); |
There was a problem hiding this comment.
When the target group already contains an imported class with the same short name as the middleware being added, this basename comparison treats different classes as duplicates. For example, if the bootstrap imports App\Http\Middleware\Authenticate and the group contains Authenticate::class, adding Illuminate\Auth\Middleware\Authenticate::class is skipped even though it resolves to a different middleware, so the requested middleware is omitted from the saved file; compare against resolved FQCNs instead of only class_basename().
Useful? React with 👍 / 👎.
…to add-midlewares-prepend-group
refs: #73