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.

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:

Comments

facine created an issue. See original summary.

facine’s picture

Status: Active » Needs review
StatusFileSize
new27.23 KB

Attach a patch with the code.

manuel garcia’s picture

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

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

Issue summary: View changes
tr’s picture

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

Status: Needs work » Needs review
StatusFileSize
new11.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

Status: Needs work » Needs review
StatusFileSize
new11.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

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Needs a reroll

facine’s picture

Status: Needs work » Needs review
StatusFileSize
new12.88 KB

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

swirt’s picture

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

swirt’s picture

Assigned: Unassigned » swirt

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

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

This was released as 8.x-1.6

swirt’s picture

Status: Fixed » Closed (fixed)