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

[Messenger] ease testing and allow forking the middleware stack #31204

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Copy link
Member

commented Apr 23, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31179
License MIT
Doc PR -

A less radical alternative than #31185 that preserves laziness and addresses the linked issue.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:messenger-stack branch from b38a927 to 3bdf4b0 Apr 23, 2019

@weaverryan
Copy link
Member

left a comment

I played with this - works really nicely to solve the problem of a middleware calling handle() multiple times. The middleware needs to know they should clone the stack, but I think that's not a huge issue.

return $this->iterator = null;
}
return $this->stack[] = $this->iterator->current();

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 23, 2019

Member

I had to stare at this for a minute. When you clone the StackMiddleware, each clone will still maintain this same, one instance of this MiddlewareStack, but will now maintain their own StackMiddleware.offset property (so, that property will move independently). The first StackMiddleware that uses an item from the iterator will use it, but store it on the stack so that it's available when the other StackMiddleware asks for that same $offset.

/**
* @internal
*/
class MiddlewareStack

This comment has been minimized.

Copy link
@danizord

danizord Apr 23, 2019

Can't you simply drop the iterator, store middleware as array in StackMiddleware and advance the pointer manually?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 23, 2019

Author Member

That would defeat laziness, which is a desired property.

@nicolas-grekas nicolas-grekas merged commit 3bdf4b0 into symfony:master Apr 26, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Apr 26, 2019

feature #31204 [Messenger] ease testing and allow forking the middlew…
…are stack (nicolas-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] ease testing and allow forking the middleware stack

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31179
| License       | MIT
| Doc PR        | -

A less radical alternative than #31185 that preserves laziness and addresses the linked issue.

Commits
-------

3bdf4b0 [Messenger] ease testing and allow forking the middleware stack

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:messenger-stack branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.