Incoming message handling tweaks by dpi · Pull Request #60 · dpi/smsframework

dpi · 2016-12-03T06:11:37Z

https://www.drupal.org/node/2832601

Added results and reports for incoming messages created in tests. Added createMessageResult test helper to create reports and result for a message.

Compacted sms_test_gateway gateway config schema (Derivative types) Added plugin incoming method to execution order tests. New test file covering \Drupal\sms\EventSubscriber\SmsMessageProcessor (some \Drupal\Tests\sms\Kernel\SmsFrameworkProviderTest tests should be moved there)

Added SmsGateway::supportsIncoming() Add preprocess check to incoming messages ensuring they have a gateway set, and the gateway supports incoming messages. Added tests for above. Added missing gateway entity to incoming message tests.

almaudoh · 2016-12-10T14:43:54Z

-
-      $this->dispatchEvent(SmsEvents::MESSAGE_INCOMING_POST_PROCESS, [$sms_message]);
-      $this->dispatchEvent(SmsEvents::MESSAGE_POST_PROCESS, [$sms_message]);
+    if ($plugin instanceof SmsGatewayPluginIncomingEventInterface) {


What I did not understand here is how a plugin can also be an Event. What is the reasoning behind the interface name?
Ok, I just checked the SmsGatewayPluginIncomingEventInterface interface description and so I think it should be called IncomingEventSubscriberInterface or IncomingEventProcessorInterface so it's clearer that the plugin is an incoming event subscriber or processor.

This interface is designed specifically for gateway plugins, do we not want to prefix these interfaces with SmsGateway or SmsGatewayPlugin?

SmsGatewayPluginIncomingEventInterface (current, prefixed)

SmsGatewayPluginIncomingEventSubscriberInterface

SmsGatewayPluginIncomingEventProcessorInterface I'd probably go with this one.

Bit of a mouthful 😒

It's the verbosity that made me leave out 'SmsGatewayPlugin..' in my original suggestion. These Drupal interface names are becoming like Java class names :)

TBH I can't really decide which is better: SmsGatewayPluginIncomingEventProcessorInterface is too long, but it's also consistent with others. So it depends on how people feel about long class and interface names. Maybe it's becoming acceptable in the Drupal world.
OTOH, since IncomingEventProcessorInterface is namespaced in Drupal\sms\Plugin it should be understood that it's specifically for gateway plugins.

since IncomingEventProcessorInterface is namespaced in Drupal\sms\Plugin it should be understood that it's specifically for gateway plugins.

Not quite. I think if it was in \Drupal\sms\Plugin\SmsGateway we can avoid the prefix.

Result:

\Drupal\sms\Plugin\SmsGateway\IncomingEventProcessorInterface

Renaming to \Drupal\sms\Plugin\SmsGateway\SmsIncomingEventInterface

It would be more consistent with the rest of plugin interfaces to have the directory name as the file prefix (SmsGatewayIncomingEventInterface).

File name still has the module prefix. Its in the SmsGateway directory so we can omit 'Gateway' I suppose. 'IncomingEvent' portion of file name matches the method exactly incomingEvent.

almaudoh · 2016-12-10T15:15:21Z

+      }
+
+      // Slice of the first {$delete_reports}x reports.
+      $reports = array_slice($reports, $delete_reports);


This should be "// Slice off..."

almaudoh · 2016-12-10T15:25:08Z

+  }
+
+  /**
+   * Ensure exception thrown if incoming message is missing a result.


Description needs update.

almaudoh · 2016-12-10T15:29:35Z

    $this->container->get('cron')->run();

    $expected[] = SmsEvents::MESSAGE_INCOMING_PRE_PROCESS;
+    $expected[] = 'incomingEvent'; // Plugin incoming event.


It looks wierd that this is the only one without a constant defined in SmsEvents? Is it not a real event like the others?

Is it not a real event like the others?

Not exactly, see \Drupal\sms\Provider\DefaultSmsProvider::incoming

https://www.drupal.org/node/2832601#comment-11809897: Comment 7 & 8 are pretty much why this is weird.

I think we should use a constant like the others for consistency sake. Even though it's not a subscribed Symfony event like the others. The purpose of a constant is to store a value so you remain DRY in your programming.
So even this is not a real event, it needs to live somewhere as a constant and I don't think we need to create another class just for it.

I think we should use a constant like the others for consistency sake. Even though it's not a subscribed Symfony event like the others. The purpose of a constant is to store a value so you remain DRY in your programming.

Except this value 'incomingEvent' is ONLY used for testing, and only in the sms_test_gateway submodule. The events are used in normal operation of the module.

The 'incomingEvent' is almost constant-like in its meaning, because it is derived from the method name. It isnt set from a plain string. See \Drupal\sms_test_gateway\Plugin\SmsGateway\Memory::incomingEvent

I'm not saying the $expected[] = 'incomingEvent'; assertion is aesthetically good. But if we could refer to method names in PHP like we can with classes ($class::class - class name resoution) then I would do that here.

Since we are using in tests only, I guess I can live with that.

Before this is committed I'm going to change the magic constant from __FUNCTION__ to __METHOD__ so it captures the full namespace and class name. So it is easier to reverse engineer where its coming from when looking at the testing/assertion code.

$expected[] = 'incomingEvent'; will become $expected[] = 'Drupal\sms_test_gateway\Plugin\SmsGateway\Memory::incomingEvent'

…rface

almaudoh · 2016-12-15T11:51:17Z

+/**
+ * Interface for gateways implementing an incoming event subscriber.
+ */
+interface SmsIncomingEventInterface {


Still missed out the 'Processor' part which I think is the key word here SmsIncomingEventProcessorInterface or SmsIncomingEventSubscriberInterface. I guess "Processor" is more preferred to avoid confusion with real Symfony event subscribers.

SmsIncomingEventInterface renamed to SmsIncomingEventProcessorInterface

Made memory chunked gateway accept incoming for the tests that ensure incoming messages don’t get chunked

#60 (comment)

…sIncomingEventProcessorInterface;

WIP - Enforce result and reports && Plugin incoming method

1fc03f8

dpi force-pushed the incoming-tweaks-2832601 branch from 5f6cf34 to 1fc03f8 Compare December 3, 2016 06:13

Fixed tests

6c1edaa

Added results and reports for incoming messages created in tests. Added createMessageResult test helper to create reports and result for a message.

dpi force-pushed the incoming-tweaks-2832601 branch from a6702a9 to cc2f4cf Compare December 3, 2016 09:13

Fixed failing tests due to missing reports

d8c3170

dpi force-pushed the incoming-tweaks-2832601 branch from cc2f4cf to d8c3170 Compare December 3, 2016 10:46

dpi force-pushed the incoming-tweaks-2832601 branch from e0c168c to 9f18f1f Compare December 4, 2016 10:22

dpi force-pushed the incoming-tweaks-2832601 branch from 9f18f1f to ffbe6a1 Compare December 4, 2016 10:42

almaudoh reviewed Dec 10, 2016

View reviewed changes

dpi added 3 commits December 11, 2016 14:13

Addressed some documentation nits from first review.

f152bfb

Rename SmsGatewayPluginIncomingEventInterface to SmsIncomingEventInte…

adb8ce7

…rface

Merge branch '8.x-1.x' into incoming-tweaks-2832601

4c61097

almaudoh reviewed Dec 15, 2016

View reviewed changes

dpi added 4 commits December 15, 2016 20:42

Fixed some failing tests.

f88fdc4

Made memory chunked gateway accept incoming for the tests that ensure incoming messages don’t get chunked

Assert full namespace+classname+method instead of just method name.

9f80bcd

#60 (comment)

Renamed \Drupal\sms\Plugin\SmsGateway\SmsIncomingEventInterface to Sm…

957cf52

…sIncomingEventProcessorInterface;

Merge branch '8.x-1.x' into incoming-tweaks-2832601

b54b459

dpi force-pushed the incoming-tweaks-2832601 branch from 2612c34 to b54b459 Compare December 15, 2016 13:22

dpi closed this Dec 15, 2016

Conversation

dpi commented Dec 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

almaudoh Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpi Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

almaudoh Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpi Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

almaudoh Dec 11, 2016 •

edited

Loading

dpi Dec 15, 2016 •

edited

Loading

almaudoh Dec 15, 2016 •

edited

Loading

dpi Dec 15, 2016 •

edited

Loading