- Download Diff

Summary:

Vortex: add ApnsPHP dispatcher

Review Request #446 — Created May 26, 2016 and submitted July 5, 2016, 10:31 a.m.

Information
Owner:	tardypad
Repository:	Lunr
Branch:	apns_php_dispatcher
Bugs:
Depends On:	~~447~~
Reviewers
Groups:	lunr
People:

Description

The new ApnsPHP is pretty annoying to work with inside our notification structure in the end...
see http://reviews.lunr.nl/r/447/ as well for the response class

It's not straightforward to define the status of the push for each endpoint:
- the library only returns an error structure (in a weird format...) for the endpoints with a failure
- some invalid endpoints are detected early on before the push and we don't get them as part of this report (because they are detected in a different Message class)
- we can then assume that the notification was delivered successfully to the endpoints left

Testing Done

unit tests

Issues

0
0
3
0
All Issues: 3

Description	From	Last Updated
Not necessarily. It's a bit ugly since we have to honor Dependency Injection, but rather then pass the Message object ...	pprkut	June 14, 2016, 1:29 p.m.
I'd prefer use statements on top rather than absolute references in the code. Makes it slightly easier to see external ...	pprkut	June 23, 2016, 4:02 p.m.
why do we need this? I'd say this has no business here. The user can configure ApnsPHP however he/she wants ...	pprkut	June 23, 2016, 4:02 p.m.

Change Summary:

multi dispatcher interface

Diff:

Revision 2 (+203)

Show changes

src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php

Re Logging, it might be worth switching ApnsPHP over to PSR-3.
Their logger interface is more or less compatible already.

tardypad June 14, 2016, 1:46 p.m.

I'm fine with moving ApnsPHP to PSR-3 but I'm not sure it will be that useful.

Here below is an example of all that the library is logging internally for a sending to only 14 endpoints, including a few invalid ones.


Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Trying tls://gateway.push.apple.com:2195...
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Connected to tls://gateway.push.apple.com:2195.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Sending messages queue, run #1: 14 message(s) left in queue.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 1 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 2 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 3 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 4 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 5 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 6 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 7 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 8 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 9 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 10 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 11 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 12 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 13 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 14 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: ERROR: Unable to send message ID 1: Invalid token (8).
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Disconnected.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Trying tls://gateway.push.apple.com:2195...
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Connected to tls://gateway.push.apple.com:2195.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: INFO: Sending messages queue, run #2: 14 message(s) left in queue.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: WARNING: Message ID 1 [custom identifier: unset] has an unrecoverable error (8), removing from queue without retrying...
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 2 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 3 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 4 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 5 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 6 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:27 +0200 ApnsPHP[24063]: STATUS: Sending message ID 7 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 8 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 9 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 10 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 11 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 12 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 13 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 14 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: ERROR: Unable to send message ID 13: Invalid token (8).
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: INFO: Disconnected.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: INFO: Trying tls://gateway.push.apple.com:2195...
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: INFO: Connected to tls://gateway.push.apple.com:2195.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: INFO: Sending messages queue, run #3: 2 message(s) left in queue.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: WARNING: Message ID 13 [custom identifier: unset] has an unrecoverable error (8), removing from queue without retrying...
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: STATUS: Sending message ID 14 [custom identifier: unset] (1/3): 80 bytes.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: ERROR: Unable to send message ID 14: Invalid token (8).
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: INFO: Disconnected.
Tue, 14 Jun 2016 13:27:28 +0200 ApnsPHP[24063]: INFO: Trying tls://gateway.push.apple.com:2195...
Tue, 14 Jun 2016 13:27:29 +0200 ApnsPHP[24063]: INFO: Connected to tls://gateway.push.apple.com:2195.
Tue, 14 Jun 2016 13:27:29 +0200 ApnsPHP[24063]: INFO: Sending messages queue, run #4: 1 message(s) left in queue.
Tue, 14 Jun 2016 13:27:29 +0200 ApnsPHP[24063]: WARNING: Message ID 14 [custom identifier: unset] has an unrecoverable error (8), removing from queue without retrying...
Tue, 14 Jun 2016 13:27:30 +0200 ApnsPHP[24063]: INFO: Disconnected.


Now I am not sure we really want all this when sending to a few thousands of them.

What would be more useful would be a way to disable their logging altogether, and just use ours build around.

Or if we implement PSR-3, we would need a way to specify the log level we want to have.

smillernl June 14, 2016, 1:56 p.m.

But implementing PSR-3 would give us exactly that option right? To specify a loglevel?

tardypad June 14, 2016, 2:38 p.m.

Not directly I think.

It would remove the need for their own interface https://github.com/immobiliare/ApnsPHP/blob/master/ApnsPHP/Log/Interface.php
and it would replace their calls $this->_log('ERROR: Unable to send message ID) by $this->_log->error('Unable to send message ID) for example.
But all those individual loggings would still be there in the library.
There is nothing there that would say "Only log starting from the error level".

This behavior would need to be implemented afterwards within the library itself or within the logger we would provide

pprkut June 14, 2016, 2:47 p.m.

Typically this is implemented in the logger. It's not zero-cost then, but depending on how it's implemented it might be close to it.
We could for example easily write a FilterLogger for Lunr that just ignores errors of certain levels (we can't do stuff like $level > NOTICE though, thank you PSR-3!). Application developers could then even selectively use that FilterLogger only for ApnsPHP (by means of our ServiceLocator or some similar such).
There is also the choice of Monolog's FilterHandler, which would essentially do the same thing, but I don't know enough about that one.

tardypad June 14, 2016, 2:57 p.m.

Yes I was thinking of something like this

The question is: do we need any of the log that are there?
I don't see anything more useful than what we would log ourselves in the dispatcher and response classes
In that case, it would be simpler to just add a functionnality to disable the logging.

pprkut June 14, 2016, 3:32 p.m.

With PSR-3 we'd already have that. We could use our NullLogger to just ignore all log messages.

tardypad June 14, 2016, 3:37 p.m.

Indeed I didn't think of this one
Well I guess, we need to go for implementing PSR3 there then

src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php (Diff revision 2)

Show all issues

Not necessarily. It's a bit ugly since we have to honor Dependency Injection, but rather then pass the Message object in the constructor, we can re-instantiate a new one on every call to push().

We can do this by implementing a get_new_apns_message() method that returns the object. That way we can mock it (sort of).

Change Summary:

add get_new_apns_message function

Description:

		The new ApnsPHP is pretty annoying to work with inside our notification structure in the end...
		see http://reviews.lunr.nl/r/447/ as well for the response class

		It's not straightforward to define the status of the push for each endpoint:
		- the library only returns an error structure (in a weird format...) for the endpoints with a failure
		- some invalid endpoints are detected early on before the push and we don't get them as part of this report (because they are detected in a different Message class)
		- we can then assume that the notification was delivered successfully to the endpoints left

-		We would need to implement a reset function in the library for the message class (see comment inside diff)
-
		And we have as well a problem with the logger
		We can't use one of our loggers without making it implement their simple ApnsPHP_Log_Interface.
		But we don't really want it anyway since the library logging is really really verbose and we can't control it
		Unfortunately using their default provided logger seems to break things when used within a Resque job...
		Need more research on that.

Diff:

Revision 3 (+208)

Show changes

src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php

Change Summary:

minor fixes
add unit tests

Description:

		The new ApnsPHP is pretty annoying to work with inside our notification structure in the end...
		see http://reviews.lunr.nl/r/447/ as well for the response class

		It's not straightforward to define the status of the push for each endpoint:
		- the library only returns an error structure (in a weird format...) for the endpoints with a failure
		- some invalid endpoints are detected early on before the push and we don't get them as part of this report (because they are detected in a different Message class)
		- we can then assume that the notification was delivered successfully to the endpoints left
-
-		And we have as well a problem with the logger
-		We can't use one of our loggers without making it implement their simple ApnsPHP_Log_Interface.
-		But we don't really want it anyway since the library logging is really really verbose and we can't control it
-		Unfortunately using their default provided logger seems to break things when used within a Resque job...
-		Need more research on that.

Testing Done:

~		example notifications sent
	~	unit tests

Depends On:

+	~~447 - Vortex: add ApnsPHP response~~

Diff:

Revision 4 (+742)

Show changes

	src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherBaseTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherPushTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherSetTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherTest.php
	tests/phpunit.xml

Change Summary:

add set_internal_logger function

Diff:

Revision 5 (+790)

Show changes

	src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherBaseTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherPushTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherSetTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherTest.php
	tests/phpunit.xml

src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php (Diff revision 5)

Show all issues

I'd prefer use statements on top rather than absolute references in the code. Makes it slightly easier to see external dependencies.

src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php (Diff revision 5)

Show all issues

why do we need this?

I'd say this has no business here. The user can configure ApnsPHP however he/she wants before (or even after) passing it to the constructor of this class.

Change Summary:

put ApnsPHP class in use statements

remove set_internal_logger function

Diff:

Revision 6 (+745)

Show changes

	src/Lunr/Vortex/APNS/ApnsPHP/APNSDispatcher.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherBaseTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherPushTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherSetTest.php
	src/Lunr/Vortex/APNS/ApnsPHP/Tests/APNSDispatcherTest.php
	tests/phpunit.xml

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

Vortex: add ApnsPHP dispatcher

Change Summary:

Diff:

Change Summary:

Description:

Diff:

Change Summary:

Description:

Testing Done:

Depends On:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: