Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds senderid configuration and process to allow feedback loops to t… #547

Merged
merged 9 commits into from Jun 7, 2019

Conversation

Projects
None yet
5 participants
@utagawal
Copy link
Contributor

commented May 29, 2019

…rigger

Description

Implements simple recommendation from https://support.google.com/mail/answer/6254652?hl=en
to allow Google mail feedback loop process to work

Related Issue

Allow phplist to be compliant with gmail feedback loop service

Screenshots (if appropriate):

@utagawal utagawal changed the title Adds senderid configuration and preocess to allow feedback lopps to t… Adds senderid configuration and preocess to allow feedback loops to t… May 29, 2019

@utagawal utagawal changed the title Adds senderid configuration and preocess to allow feedback loops to t… Adds senderid configuration and process to allow feedback loops to t… May 29, 2019

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Thanks! Looks good. Regarding the senderid, that appears to be a hardcoded string, but shouldn't it be a different value for each host or installation? In which case it should probably be easier to set than by editing that hardcoded value. What is the impact of using this new header including literally the string 'senderid'? Presumably the feedback loop would fail.

@utagawal

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I let the core team decide how they want to pass on the config Item as I'm not sure about the strategy here (config file or Database)

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@utagawal OK, so you confirm it needs to be set independently for each installation in order for the feedback loop to work with most mail hosts?

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@utagawal Would it be better to leave senderid as an empty string, and add a conditional to check if it is set before using the feedback loop header? Seems to me that would be safer than enabling the header with an invalid value by default.

@samtuke

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@utagawal The easiest way to make the var configurable is to use a new config option in the config.php / config_extended.php file. That is simple to do and acheives the goal of easy configuration and documentation for users.

@michield

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Yes, it's a good idea, but needs some more detail to work as a feature that can be accepted.

The correct way to make this change is as follows:

  1. make $senderid a constant, eg SENDERID. As a variable, it's too dangerous as it might change halfway the code.

As this is Google specific, it is probably even better to call it GOOGLE_SENDERID

  1. Initialise the constant in init.php with an "if not defined" and initialise it to the empty string

  2. add the constant to config_extended with an explanation

  3. add the header based on "if not empty" so that it's not added when the senderid is not defined.

Bonus points would be if the "a : b : c" from https://support.google.com/mail/answer/6254652?hl=en are implemented, ie "campaign ID", "subscriber ID" etc.

More bonus points would be to document it on the resources wiki.

@suelaP

This comment has been minimized.

Copy link
Member

commented May 30, 2019

@utagawal how do you feel about implementing those recommendations? This looks like a good improvement to be included in the next release. Someone from our team is willing to check it tomorrow if you are not planning to and you are ok with that :)

@utagawal

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

As said I'm no real developer. I have followed Michiel's explanation. But didn't go for the extra mile as I'm not sure how/where to gather the campaingId and subscriber ID variables.
It should be quite straight forward for a developer in the core team to extend it though...

utagawal added some commits May 31, 2019

Add GOOGLE_SENDERID as a CONSTANT in config_extended, initialized it …
…and add a check to ensure it is defined bedoire adding the line in the message header
Show resolved Hide resolved public_html/lists/admin/class.phplistmailer.php Outdated
Show resolved Hide resolved public_html/lists/config/config_extended.php Outdated
Show resolved Hide resolved public_html/lists/config/config_extended.php Outdated
@utagawal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2019

Can you let me know how to ge the campaingId and subscriber ID variables so I can add them to the message header too as requested ?

@michield

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

Looks good to me. @xh3n1 ?

@utagawal don't worry about the abc variables. That can be added later. As it stands, this will work.

@xh3n1

xh3n1 approved these changes Jun 2, 2019

@samtuke

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@utagawal Once you apply Xheni's change I'll merge

@suelaP

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@utagawal any blocks on getting @xh3n1 recommendations?

@utagawal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@suelaP I have implemented the recommended change proposed by @xh3n1

@xh3n1

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@utagawal The test is failing now because you should remove the quotes in !empty('GOOGLE_SENDERID') :)

@utagawal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@xh3n1 fixed

@xh3n1

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@utagawal, it seems that this syntax is not supported in PHP 5.4, sorry about that.
In this case, please remove !empty('GOOGLE_SENDERID') leave it as: if (GOOGLE_SENDERID != '') since the constant is defined anyway in init.php.

Thanks.

@utagawal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

ok resolved

@xh3n1

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@utagawal Great, thanks 😃

@samtuke samtuke merged commit 384bbf0 into phpList:master Jun 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@samtuke

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@utagawal Please can you take 2 minutes to agree to this CLA? Unfortunately without that we cannot ship this change in phpList 3.4.3. You need do it only once: https://phplist.com/cla

@samtuke

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

@utagawal Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.