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.

Sign up for GitHub

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

Jump to bottom

Messenger support #1360

Merged
merged 4 commits into from
Oct 12, 2021
Merged

Messenger support #1360

merged 4 commits into from
Oct 12, 2021

Conversation

mynameisbogdan
Copy link
Contributor

@mynameisbogdan mynameisbogdan commented Mar 4, 2021 •
edited
Loading

Q A
Branch? 2.x
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1193
License MIT
Doc PR

Adding optional symfony/messenger support.

@mynameisbogdan mynameisbogdan marked this pull request as draft March 4, 2021 19:34
@coveralls
Copy link

coveralls commented Mar 4, 2021 •
edited
Loading

Coverage Status

Coverage decreased (-0.08%) to 84.043% when pulling e6ba79e on mynameisbogdan:messenger-support into 45f09d8 on liip:2.x.

@mynameisbogdan mynameisbogdan mentioned this pull request Mar 10, 2021
Closed
@peter-gribanov
Copy link
Contributor

Perhaps you can use the EnqueueBundle documentation as a basis to document this feature.

@alex-cool-tech
Copy link

Hi, any updates maybe?

dbu
dbu approved these changes Oct 5, 2021
View reviewed changes
Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot, this looks good!

can you please add documentation (duplicate and adjust what we have for enqueue-bundle)?

then we can merge this, and we will deprecate the enqueue integration.

dbu
dbu reviewed Oct 5, 2021
View reviewed changes
Message/Handler/ResolveCacheHandler.php Outdated

foreach ($filters as $filter) {
if ($message->isForce()) {
$this->filterService->bustCache($path, $filter);
Copy link
Member

Choose a reason for hiding this comment

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

likely needs to be adjusted to the change in #1347 once that is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change it then.

This was referenced Oct 5, 2021
Merged
Closed
Closed
Closed
@mynameisbogdan mynameisbogdan changed the title [WIP] Messenger support Messenger support Oct 6, 2021
@mynameisbogdan mynameisbogdan marked this pull request as ready for review October 6, 2021 09:55
@mynameisbogdan
Copy link
Contributor Author

mynameisbogdan commented Oct 6, 2021 •
edited
Loading

@dbu done regarding the docs.

dbu
dbu approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot, this looks good!

i have some suggestions, and please make the underlines match the length of the titles in the .rst doc.

i am a bit unhappy with the naming of "ResolveCache" when what we really do is warm up the cache. resolve is the word in the loaders, and it has the side effect of filling the cache. the unelegant terminology is already used with enqueue, but i intend to deprecate that enqueue integration once your PR is merged. what would you think of renaming the message to WarmupCache and the handler to CacheWarmupHandler?

@mynameisbogdan mynameisbogdan force-pushed the messenger-support branch 2 times, most recently from 681fdbe to 37e7285 Compare October 7, 2021 09:34
@mynameisbogdan
Copy link
Contributor Author

mynameisbogdan commented Oct 7, 2021 •
edited
Loading

Great @dbu!

I implemented your suggestions with one little change. I renamed the message to WarmupCache, and the handler to WarmupCacheHandler to stick to Symfony's naming convention of ${messageClassName}Handler.

Also in the docs, I changed some resolve wording to warmup. And recommended a new transport named liip_imagine.

It's this okay with you?

dbu
dbu approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot!

@dbu dbu merged commit 7282ab2 into liip:2.x Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbu dbu dbu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mynameisbogdan @coveralls @peter-gribanov @alex-cool-tech @dbu