Message Broker Integration [#1831250]

Description: This is a module intended for developers that want to use a message broker in combination with drupal. It allows you to send and receive messages without caring about the underlying technical challenges.

Possible use cases include synchronizing drupal sites, integrating external systems, implementing distributed business logic or seperating heavy processing tasks from your normal drupal requests.

Project page: Message Broker

Repo: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sirup/1807310.git message_broker

Intended for Drupal 7

Similar modules

There are two similar modules: RabbitMQ and AMQP. This are the differences:

both modules are tied to amqp, this module is more generic and would allow to plugin other protocols like STOMP (the dummy module proves that this is already possible)
this module stores the routing configuration in simple json files, allowing to reuse the file in other non-drupal applications (which is essential in many distributed systems)
we allow to test AMQP messaging in-memory via our unique dummy module
amqp module in detail
- the module is only available for Drupal 6.x
- it adapts the Drupal Queue interface, which has its advantages but also throws away the power of the AMQP concepts: AMQP allows much more powerful things than queuing
- even the queueing implementation is not finished, because the drupal queue interface can not be mapped easily on the AMQP concepts
- besides the name, it actually uses the RabbitMQ product directly via a http interface that is not available when using other amqp products
rabbitmq module in detail
- the module also adapts the queue interface from drupal with the same limitations
- not usable in combination with non-PHP systems or legacy systems as it has many assumptions about the structure of messages and exchanges

Conclusion: Both modules do provide only queuing functionality on top of Drupals queue interface. In contrast this module is a full messaging solution that could be used for queueing, but is not limited to this use case. See the project page for more examples.
In fact the module names are misleading, because the amqp module is tied to rabbitmq and the rabbitmq one is more generic than the name states.

Reviews of other projects
http://drupal.org/node/1840698#comment-6739492
http://drupal.org/node/1842792#comment-6744726
http://drupal.org/node/1840692#comment-6739522

http://drupal.org/node/1842792#comment-6741930
http://drupal.org/node/1882080#comment-6915780, http://drupal.org/node/1882000#comment-6915796 and http://drupal.org/node/1877836#comment-6915840
http://drupal.org/node/1880120#comment-6915908

http://drupal.org/node/1889164#comment-6942104

Thanks in advance

Comment	File	Size	Author
#18	coder-results.txt	6.96 KB	klausi
#5	amqp-consume.png	34.41 KB	sirup

Comments

Comment #1

sirup commented 3 November 2012 at 23:37

One note to the http://ventral.org/pareview/ script: There seem to be many false positives which pop up when analyzing this modules code. For instance it does not recognize nested sub modules.

Comment #2

dDoak commented 4 November 2012 at 17:13

Status:

Needs review

» Needs work

Hi,

A lots of work have to be done according to this report :
http://ventral.org/pareview/httpgitdrupalorgsandboxsirup1807310git ?

Manual review :
A great idea to place configuration in json.

Why not using drupal autoloader? DB calls?

For example :
If you add this in dot info file:
files[] = message_broker_exception.inc

This function will become useless :

function message_broker_integration_throw_critical_error($message = NULL, $code = NULL, $previous = NULL) {
  // we include the file manually to prevent the drupal autoloading. which
  // uses DB calls
  require_once 'message_broker_exception.inc';

  return new CriticalErrorException($message, $code, $previous);
}

In message_broker_amqp_integration module, files are not correctly named (prefixed with module name) : amqp_message_broker.inc

Comment #3

sirup commented 15 January 2013 at 09:20

Status:

Reviewed & tested by the community

» Needs work

~~I didnt knew that the Drupal autoloader does not issue a DB call when the file is listed within the .info file. I will try and change it.~~ Update: my asumption was true, the drupal autoload uses a database table to autoload classes

The reason for the seperate method was that we were required to throw the exception even when the database connection was lost. This may occur when you run an AMQP consumer via drush for a long time. The standard behavior of mysql is to kill the connection after a large amount of idle time.

Will fix the other problems the next days.

Thanks

Comment #4

sirup commented 6 November 2012 at 13:38

Status:

Needs work

» Needs review

I fixed all Coder fixes and warnings except the following false positives:

function name prefixes are correct, the automatic review does not recognize the sub modules in the "modules" folder
drupal_set_message() input is filtered, the argument is the return value of the l() function
Some arguments for the l() function are not wrapped within a t() call, because the argument is a (non translatable) url
Line indentation errors occur where we use a PHP 5.3 closure, which seems to be unknown to the review module

Furthermore i re-added the helper method message_broker_integration_throw_critical_error and explained in the comment why: It allows to create the exception instance even when the DB connection was lost. Therefore we cant rely on Drupals autoloading which reads from the registry_files table.

Comment #4.0

sirup commented 12 November 2012 at 08:28

Issue summary:

View changes

compare this module with similar ones

Comment #5

sirup commented 12 November 2012 at 09:15

Status	File	Size
new	amqp-consume.png	34.41 KB

Reviewing this module is a bit difficult, because it requires to install external libraries and third party software. Therefore we have written a step-by-step list which explains how to test the two modules:

Message Broker Dummy Integration module

Enable "Message Broker Dummy Integration Modul" and "Message Broker Integration Example" module
Goto admin/config/system/message_broker_integration and choose the dummy implementation
Goto admin/config/system/message_broker_integration_example and test the form to see the dummy implementation in action

When typing "Georg" for instance, you should see the output

Message sent to exchange "helloWorld" with routing key "helloWorld.Georg"
Start consuming a message with "helloWorldToEveryone"
Hello world, Georg
Message was processed successfully by consumer "helloWorldToEveryone"

Message Broker AMQP Module

Install RabbitMQ http://www.rabbitmq.com/download.html (and Erlang if missing)
Download dependencies for AMQP module (xautoload, libraries)
Enable "Message Broker AMQP Integration" module
Install PHP AMQP library from https://github.com/videlalvaro/php-amqplib to libraries folder
Go to admin/config/system/message_broker_integration and choose the AMQP implementation
Enable the "Message Broker Integration Example" module
Go to admin/config/system/message_broker_amqp_integration and ensure the config path is set to sites/all/modules/message_broker_integration/modules/message_broker_integration_example/example_config.json
Start RabbitMQ (i.e. run rabbitmq-server.bat script)
Go to example admin/config/system/message_broker_integration_example and send a few messages (you will not see a direct feedback)
Start the consumer via drush: drush consume-amqp-messages
You should see the output of the messages you sent in 9 (see amqp-consume.png attachment).

This should ease the review hopefully.

Comment #5.0

sirup commented 12 November 2012 at 09:15

Issue summary:

View changes

fix typos

Comment #6

sirup commented 17 November 2012 at 17:37

Issue tags:

+PAreview: review bonus

adding review bonus tag

Comment #7

grisendo commented 17 November 2012 at 21:47

Status:	Needs review	» Needs work
Issue tags:	-PAreview: review bonus

There are still many line indentation errors.
i.e.: message_broker_amqp_integration.install, line 28: ERROR | Line indented incorrectly; expected 4 spaces, found 6

There are also some strings form_set_error messages without t():
i.e.: message_broker_amqp_integration.module, lines 178 and 186.

Removing review bonus tag, after fixing you can add it again if you have done another 3 reviews of other projects.

Comment #8

sirup commented 17 November 2012 at 22:05

Status:	Needs work	» Needs review
Issue tags:		+PAreview: review bonus

There are still many line indentation errors.

Please read all posts carefully before raising issues discussed already above. See comment #4.

Following other reviews i re-add the tag as the removal requires an in-depth review. Therefore please try to use the module and follow the steps given above to test all functionality. Small formatting issues are not cricitical and are no reason to set the state back to "needs work".

Comment #9

grisendo commented 17 November 2012 at 22:33

Status:	Needs review	» Needs work
Issue tags:	-PAreview: review bonus

I made a in-depth review, I followed all the steps and tested both Dummy and AMQP, and everything went OK for me. So I started to try security hacks, and check code line by line...
I also read all previous comments, so I didn't say anything about that false positives you say.
I just informed about that two lines with the extra spaces, that are not part of your "false positives".

The reason I change back to "needs work" and removed "pareview: bonus review" tag is because you didn't use the t() function in that form_set_error messages:
http://drupal.org/node/1187664

Otherwise, you can wait for days to a git admin that will review the project and ask for that changes... and wait more days for the git admins to re-review your project.

Comment #10

sirup commented 17 November 2012 at 23:07

Status:	Needs work	» Needs review
Issue tags:		+PAreview: review bonus

I followed all the steps and tested both Dummy and AMQP, and everything went OK for me.

Great!

The reason I change back to "needs work" and removed "pareview: bonus review" tag is because you didn't use the t() function in that form_set_error messages:

Ok. This is fixed now.

If nothing else popus up, we can switch to "reviewed & tested by the community" :-)

Comment #11

sirup commented 17 November 2012 at 23:08

Issue tags:

-PAreview: review bonus

remove tag that was added accidently

Comment #12

sirup commented 3 December 2012 at 22:23

Priority:

Normal

» Major

Comment #13

sirup commented 3 December 2012 at 22:30

update priority

Comment #14

monymirza

Urdu

Islamabad

commented 4 December 2012 at 07:13

Priority:	Major	» Normal
Status:	Needs review	» Needs work

What about errors/warnings pending at http://ventral.org/pareview/httpgitdrupalorgsandboxsirup1807310git ?
please follow drupal code standards.

Comment #15

sirup commented 11 December 2012 at 10:20

Priority:	Normal	» Major
Status:	Needs work	» Needs review

@monymirza: Please read the whole issue before posting comments!

Sorry, but these are false positives. This is an automated tool, so its not uncommon to have warnings and errors that are not relevant or simply not true.

Comment #16

sirup commented 19 December 2012 at 08:34

Priority:

Major

» Critical

raising the priority as no review was done for more than four weeks

Comment #16.0

sirup commented 19 December 2012 at 08:51

Issue summary:

View changes

add links to reviewed projects

Comment #16.1

sirup commented 8 January 2013 at 14:22

Issue summary:

View changes

add more code reviews

Comment #16.2

sirup commented 8 January 2013 at 14:27

Issue summary:

View changes

add link to anther comment

Comment #17

sirup commented 8 January 2013 at 14:44

Issue tags:

+PAreview: review bonus

adding review bonus tag

Comment #18

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 10 January 2013 at 22:49

Priority:	Critical	» Normal
Status:	Needs review	» Reviewed & tested by the community

Status	File	Size
new	coder-results.txt	6.96 KB

Review of the 7.x-1.x branch:

Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

message_broker_integration.module: I would recommend to rename that to simply "message_broker". "integration" seems just too verbose and all your submodules do not match the main module prefix.
"drupal_set_message('A special hello to ' . $message->name . '!');": this is vulnerable to XSS exploits. You need to sanitize all untrusted or user provided input before printing. Please read http://drupal.org/node/28984 again. Also elsewhere.
You can ignore the some of the array indentation errors, I think coder sniffer is not yet fully ready for anonymous functions.

So since the XSS exploit can only be triggered with the "administer site configuration" permission this does not qualify as security issue per http://drupal.org/security-advisory-policy . The rest is not a major blocker either, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Comment #19

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 10 January 2013 at 22:49

Issue tags:

-PAreview: review bonus

Now actually removing tag,

Comment #20

sirup commented 11 January 2013 at 13:18

Thanks for the review!

1. Renamed all modules.
2. Fixed the security problem.

Comment #21

stborchert

he/him

German

commented 12 January 2013 at 16:59

Btw., please update your account to a personal profile. At the moment it is not clear who is behind http://drupal.org/user/2321604.

All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered.

Comment #21.0

stborchert

he/him

German

commented 12 January 2013 at 16:59

Issue summary:

View changes

add the third comment

Comment #22

sirup commented 15 January 2013 at 09:24

Status:

Needs work

» Reviewed & tested by the community

Thanks for the hint. I've added the full name.

Comment #22.0

sirup commented 15 January 2013 at 09:24

Issue summary:

View changes

fix grammar

Comment #22.1

sirup commented 15 January 2013 at 09:27

Issue summary:

View changes

change module name in summary

Comment #23

sirup commented 18 January 2013 at 10:02

Fixed coder warnings (and ignoring some of the false reports) and started to work on more features (a new drush command).

Comment #24

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 18 January 2013 at 12:13

Status:

Reviewed & tested by the community

» Fixed

no other objections for more than a week, so ...

Thanks for your contribution, sirup!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.