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] Making cache rebuild correctly when message subscribers change #31445

Merged

Conversation

Projects
None yet
3 participants
@weaverryan
Copy link
Member

commented May 9, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31409
License MIT
Doc PR not needed

An edge-case that's identical to EventSubscriberInterface: when the return value of getHandledMessages() changes, the container needs to be rebuilt.

If you're wondering why these checks aren't in their own resource class, see #25984 - it's something we probably should do, but haven't done yet.

@weaverryan weaverryan force-pushed the weaverryan:messenger-subscriber-cache-rebuild branch from 2355715 to d88446b May 9, 2019

@weaverryan weaverryan changed the base branch from 4.3 to 4.2 May 9, 2019

class TestMessageSubscriberConfigHolder
{
public static $handledMessages = [];
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 9, 2019

Author Member

I discovered that testEventSubscriber() and testServiceSubscriber() are invalid. Part of the signature of ReflectionClassResource is the constants and their values. So, to purely test that the resource becomes stale when a static method is called, we must do that without modifying any constants.

Those other 2 methods need to be fixed in 3.4 - you can comment out the code in ReflectionClassResource for each and they still pass.

@weaverryan weaverryan modified the milestones: 4.3, 4.2 May 9, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Good catch, thanks @weaverryan.

@nicolas-grekas nicolas-grekas merged commit d88446b into symfony:4.2 May 11, 2019

1 of 3 checks passed

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

nicolas-grekas added a commit that referenced this pull request May 11, 2019

bug #31445 [Messenger] Making cache rebuild correctly when message su…
…bscribers change (weaverryan)

This PR was merged into the 4.2 branch.

Discussion
----------

[Messenger] Making cache rebuild correctly when message subscribers change

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

An edge-case that's identical to `EventSubscriberInterface`: when the return value of `getHandledMessages()` changes, the container needs to be rebuilt.

If you're wondering why these checks aren't in their own resource class, see #25984 - it's something we probably should do, but haven't done yet.

Commits
-------

d88446b Making cache rebuild correctly with MessageSubscriberInterface return values

@fabpot fabpot referenced this pull request May 22, 2019

Merged

Release v4.3.0-BETA2 #31577

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.