- Download Diff

Summary:

[Feedback] add mail logger

Review Request #320 — Created July 7, 2014 and discarded May 12, 2017, 11:35 a.m.

Information
Owner:	tardypad
Repository:	Lunr
Branch:	tardypad:mail_logger
Bugs:
Depends On:
Blocks:	330
Reviewers
Groups:	lunr
People:

Description

Feedback: add mail logger

Testing Done

Unit tests

Issues

0
0
13
0
All Issues: 13

Description	From	Last Updated
Not really the way we write documentation. I believe that @var Array is enough, but if you want to write ...	dinos	July 7, 2014, 4:25 p.m.
extra line :)	dinos	July 7, 2014, 4:25 p.m.
The function names in Lunr are snake case. Be sure to fix the rest as well.	dinos	July 7, 2014, 4:25 p.m.
variables also snake case, e.g. $email_to	dinos	July 7, 2014, 4:25 p.m.
too crowded. Give them a line after every semicolon.	dinos	July 7, 2014, 4:25 p.m.
What in here requires PHP 5.5?	pprkut	July 8, 2014, 5:03 p.m.
year	pprkut	July 8, 2014, 5:03 p.m.
Too vague. Please make separate parameters for to and from	pprkut	July 8, 2014, 5:03 p.m.
I see what you're trying to do here, but I don't think it's feasible to do it like this. To/From ...	pprkut	July 9, 2014, 11:17 a.m.
You generally don't want to send an email if there is no "to" set. Missing an abort case	pprkut	July 9, 2014, 3:37 p.m.
This is a bit controversial. Ideally we already have the timestamp from the email, so this is really only useful ...	pprkut	July 8, 2014, 5:03 p.m.
style :)	dinos	Aug. 1, 2014, 10:32 a.m.
Sorry, I know we had prior discussions on this and code wise it's fine. But I just found that half ...	pprkut	Aug. 22, 2014, 3:09 p.m.

Summary:

-	Feedback: add mail logger
+	[Feedback] add mail logger

src/Lunr/Feedback/MailLogger.php (Diff revision 1)

Show all issues

Not really the way we write documentation. I believe that @var Array is enough, but if you want to write more details add an extra empty line after the detailed description.

src/Lunr/Feedback/MailLogger.php (Diff revision 1)
Show all issues
```
extra line :)
```

src/Lunr/Feedback/MailLogger.php (Diff revision 1)

Show all issues

The function names in Lunr are snake case. 

Be sure to fix the rest as well.

src/Lunr/Feedback/MailLogger.php (Diff revision 1)
Show all issues
```
variables also snake case, e.g. $email_to
```

src/Lunr/Feedback/MailLogger.php (Diff revision 1)

It's not a blocking issue and it's more of a personal thing but since 5.4 we define arrays with [] instead of array()

src/Lunr/Feedback/Tests/MailLoggerBaseTest.php (Diff revision 1)

Show all issues

too crowded. Give them a line after every semicolon.

Change Summary:

Fixed problems from Dinos review

Diff:

Revision 2 (+414)

Show changes

	src/Lunr/Feedback/MailLogger.php
	src/Lunr/Feedback/Tests/MailLoggerBaseTest.php
	src/Lunr/Feedback/Tests/MailLoggerTest.php
	tests/phpunit.xml

src/Lunr/Feedback/MailLogger.php (Diff revision 2)
Show all issues
```
What in here requires PHP 5.5?
```
src/Lunr/Feedback/MailLogger.php (Diff revision 2)
Show all issues
```
year
```

src/Lunr/Feedback/MailLogger.php (Diff revision 2)

Show all issues

Too vague. Please make separate parameters for to and from

src/Lunr/Feedback/MailLogger.php (Diff revision 2)

Show all issues

I see what you're trying to do here, but I don't think it's feasible to do it like this. To/From can only be set once, so checking on every call whether they are set is pretty pointless

tardypad July 8, 2014, 5:05 p.m.

The problem is that the send function of Mail is resetting the To/From fields after a successful sending
Thus we have to define them again before every log
Or maybe we should change the Mail class itself?

pprkut July 9, 2014, 9:45 a.m.

No, changing the Mail class would break re-entrance. You don't want that.
The point here is not that you have to add the email addresses for every mail, but that you check whether they are there every time. You *know* after executing the constructor, so why do useless work?

tardypad July 9, 2014, 3:37 p.m.

Alright, then this is already fixed in the new version

src/Lunr/Feedback/MailLogger.php (Diff revision 2)

Show all issues

You generally don't want to send an email if there is no "to" set. Missing an abort case

tardypad July 8, 2014, 5:05 p.m.

The send function of the Mail class is already checking the presence of the To field before sending
What do you mean by abort case?

pprkut July 9, 2014, 9:45 a.m.

Relying on the Mail class means you still spend resources on creating a log message payload that you *know* will never be sent. So rather than doing useless work, just abort logging.

src/Lunr/Feedback/MailLogger.php (Diff revision 2)

Show all issues

This is a bit controversial. Ideally we already have the timestamp from the email, so this is really only useful if for some reason we send delayed information. That in turn should be more of a corner case, so I'd leave it up to the consumer to include that in the message.
On the other hand, controller and method seem like a good information to have in the subject, and are rather useless in the message. So maybe override compose_message() and do it slightly differently here.

Change Summary:

Fox some problems from Heinz review
- use 'to' and 'from' instead of vague 'parameters'
- change subject and message (timestamp not needed + call not in message)

Diff:

Revision 3 (+472)

Show changes

	src/Lunr/Feedback/MailLogger.php
	src/Lunr/Feedback/Tests/MailLoggerBaseTest.php
	src/Lunr/Feedback/Tests/MailLoggerTest.php
	tests/phpunit.xml

Change Summary:

improve mail logger with configuration check 
check configuration is valid before building mail

Diff:

Revision 4 (+584)

Show changes

	src/Lunr/Feedback/MailLogger.php
	src/Lunr/Feedback/Tests/MailLoggerBaseTest.php
	src/Lunr/Feedback/Tests/MailLoggerTest.php
	tests/phpunit.xml

Change Summary:

improve the mail logger tests
use of 'returnValueMap' instead of obscure 'onConsecutiveCalls'

Diff:

Revision 5 (+589)

Show changes

	src/Lunr/Feedback/MailLogger.php
	src/Lunr/Feedback/Tests/MailLoggerBaseTest.php
	src/Lunr/Feedback/Tests/MailLoggerTest.php
	tests/phpunit.xml

src/Lunr/Feedback/MailLogger.php (Diff revision 5)
Show all issues
```
style :)
```

Change Summary:

fixed one style

Diff:

Revision 6 (+591)

Show changes

	src/Lunr/Feedback/MailLogger.php
	src/Lunr/Feedback/Tests/MailLoggerBaseTest.php
	src/Lunr/Feedback/Tests/MailLoggerTest.php
	tests/phpunit.xml

Ship it!

```
Ship It!
```

src/Lunr/Feedback/MailLogger.php (Diff revision 6)

Show all issues

Sorry, I know we had prior discussions on this and code wise it's fine. But I just found that half of this is done again in the Mail class so moving this functionality there entirely (in a separate review) makes more sense IMHO

If we still want this (not sure) it would need to be ported to PHPMailer

tardypad Sept. 16, 2016, 2:13 p.m.

not used in 2 years, can be easily discarded

pprkut Sept. 16, 2016, 2:19 p.m.

well, the concept is still interesting and nothing speaks against such a feature. It would just need someone to bring it up-to-date

Status: Discarded

Change Summary:

Feedback was replaced by Monolog

You have a pending review.

Review Board 3.0.24

[Feedback] add mail logger

Summary:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Discarded

Change Summary: