Problem/Motivation

Sites with a very high traffic need to queue all messages.

Proposed resolution

  1. #3072863: Add the composer file
  2. #3170952: Configuration schema is invalid
  3. Update the slack service with two new methods 'queueMessage' and 'deliverMessage'.
  4. Define a new AdvancedQueueJobType.
  5. Update the settings form.
  6. Update the test send message form.
  7. Update the rule action.
CommentFileSizeAuthor
#13 2994023-12.patch12.88 KBfacine
#9 2994023-9.patch11.24 KBfacine
#7 2994023-7.patch11.18 KBfacine
#2 slack-queue_messages-2994023-2-8.x.patch27.23 KBfacine

Issue fork slack-2994023

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

facine created an issue. See original summary.

facine’s picture

facine commented
Status: Active » Needs review
StatusFileSize
new slack-queue_messages-2994023-2-8.x.patch27.23 KB

Attach a patch with the code.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

I think this is a great idea.

Some thoughts after a read of the patch:

  1. We should document this on the README at least, since its a new feature and has optional integration with advancedqueue by the looks of it.
  2. The patch includes a lot of cleanup which is unrelated to the proposed feature, and although they make sense, it makes it a lot harder to review.
  3. I think it may be easier if we just added basic core queues integration for now to ease the burden on maintaining this. Integrating with advancedqueue module can be done as an iteration of this if people want it. Up to the maintainers of course.

I believe these to be incorrect since advancedqueue is not a requirement:

  1. +++ b/src/Form/SettingsForm.php
@@ -2,24 +2,51 @@
+use Drupal\advancedqueue\Entity\Queue;

    This will blow up if advancedqueue is not installed. 

  2. +++ b/src/Slack.php
@@ -2,42 +2,82 @@
+use Drupal\advancedqueue\Entity\Queue;
+use Drupal\advancedqueue\Job;

    This will blow up if advancedqueue is not installed.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Issue summary: View changes
Status: Needs work » Active

Add a separate issue about the first point of Proposed resolution in summary:

(1) #3072863: Add the composer file

manuel garcia’s picture

manuel garcia commented
Issue summary: View changes
tr’s picture

tr commented
Issue summary: View changes
Status: Active » Needs work

Changed #2 to point to issue #3170952: Configuration schema is invalid

Patch needs to be re-rolled.

Why add the dependency on the contributed module advancedqueue, instead of using a core QueueWorker plugin?

facine’s picture

facine commented
Status: Needs work » Needs review
StatusFileSize
new 2994023-7.patch11.18 KB

@Manuel Garcia, thank you for reviewing this.

I'm attaching a new approach without advancedqueue dependency.

Status: Needs review » Needs work

The last submitted patch, 7: 2994023-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

facine’s picture

facine commented
Status: Needs work » Needs review
StatusFileSize
new 2994023-9.patch11.24 KB

Fixing coding standard

Status: Needs review » Needs work

The last submitted patch, 9: 2994023-9.patch, failed testing. View results

facine’s picture

facine commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Needs a reroll

facine’s picture

facine commented
Status: Needs work » Needs review
StatusFileSize
new 2994023-12.patch12.88 KB

swirt made their first commit to this issue’s fork.

swirt opened merge request !40

swirt’s picture

swirt
he/him
Primary language English
commented

I created a MR from the patch in #13 with some minor re-rolling.

swirt’s picture

swirt
he/him
Primary language English
commented
Assigned: Unassigned » swirt

  • swirt committed e2b35456 on 8.x-1.x 
    Issue #2994023 Add queue option for sending messages.
swirt’s picture

swirt
he/him
Primary language English
commented
Status: Needs review » Fixed

This has been merged. Thank you facine for keeping this dream alive for 8 years. It is a valuable improvement.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

swirt’s picture

swirt
he/him
Primary language English
commented

This was released as 8.x-1.6

swirt’s picture

swirt
he/him
Primary language English
commented
Status: Fixed » Closed (fixed)