move email header/footer where they should be

[mircobabini]

All Submissions:

• [x] Have you followed the Contributing guideline?
• [x] Does your code follow the WordPress' coding standards?
• [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Resolves the appended contents to be placed before the footer.

Other information:

• [x] Have you added an explanation of what your changes do and why you'd like us to include them?
• [x] Have you successfully run tests with your changes locally?

[ideadude]

Thanks! Going to look into this for next release. Won't make 2.7 though.

[sc0ttkclark]

This might be complicated by the kses handling which runs at priority 11 right now.

add_filter( 'pmpro_email_body', 'pmpro_kses', 11 ); is in includes/email.php and people who remove it may be removing that via remove_filter( 'pmpro_email_body', 'pmpro_kses', 11 );

Was there a specific reason on the priority change itself?

[mircobabini]

@sc0ttkclark let me explain and provide a better idea.

Was there a specific reason on the priority change itself?

At the moment (buggy) we have this messy email structure:

HEADER
BODY
FOOTER (Regards,...)
=> BODY ADDITIONS (too late!)
TEST MESSAGE (when doing tests) hooked at pmpro_email_body prio:10

What we want is:

HEADER
BODY
BODY ADDITIONS
FOOTER (Regards,...)
TEST MESSAGE (when doing tests)

When I wrote this PR the idea was: let's use a very high priority value (99999) to prepend/append header/footer to be sure we come just after any other body addition from addons or developers' code. Then at priority 100000, append the "----- TEST -----" message when we are sending a test.

Btw you are right, to high priority values here. Then you say:

This might be complicated by the kses handling which runs at priority 11 right now.

Yeah, but isn't also that priority too low? It's pretty common to see priorities like 10, 15, 20, 25 (leaving some numerical spaces) when appending texts using filters, isn't it? At least, I'm doing it sometimes 🤫 So, kses at prio 11 sounds a bit low to me.

Ideas

1. To fix the body priority mess, we can use a brand new filter pmpro_email_before_send just before the wp_mail call.
2. This can also be used to call kses at the very end of the body additions and applying kses to the header/footer as well => of course this would be a breaking change.

So this might be the final structure

PREPARE BODY pmpro_email_body => BODY ADDITIONS (only for devs and addons) pmpro_email_before_send => ADD HEADER/FOOTER pmpro_email_before_send => add "--- TEST ---" in case we are testing pmpro_email_before_send => apply kses

Which if I should say how to apply priorities, It would be:

pmpro_email_before_send, 10 => ADD HEADER/FOOTER pmpro_email_before_send, 20 => add "--- TEST ---" in case we are testing pmpro_email_before_send, 30 => apply kses

To let developers being able to do whatever they want.

[sc0ttkclark]

I like the direction we're moving based on the ideas there.

But people are already potentially removing the hook where it's at and that's something we need to determine if it's worth it as part of this solution (for them to have to go and update those snippets again).

[mircobabini]

Oh we need some creativity here!

That's another approach: https://github.com/strangerstudios/paid-memberships-pro/pull/2000/commits

• pmpro_email_body => BODY ADDITIONS (prio: 10) + KSES (prio:11) --- as it is.
• add header/footer (moved below) => also apply KSES only if the filter is still in place => this way we are not introducing a new filter to be unhooked for header/footer, but we are also keeping it separate. Just treating those parts like they were already merged into the body.

Now we still have to add the "--- TEST ---" part at the bottom. We can introduce a filter pmpro_email_before_send like suggested above: $temail = apply_filters( 'pmpro_email_before_send', $this );

BUT now we have a $temail variable once again, which should me applied to the instance ($this) in order to have the PMPro email instance reflecting what's being sent. Should we do another block of assign here?

Otherwise we can just do something like $this->body = apply_filters( 'pmpro_email_body_before_send', $this->body );

And change from add_filter('pmpro_email_body', 'pmpro_email_templates_test_body', 10, 2); to add_filter('pmpro_email_body_before_send', 'pmpro_email_templates_test_body', 20, 2); (also changed the filter from 10 to 20).

RE the kses filter at prio 11: I would still say that this approach has some limits, it's just applied too early. It's gonna be a thorn in our side forever. We must always use pmpro_email_body with prio 10, it's not permitting us and devs to do creative things with hooks. BUT of course you can add anything with prio > 11 and apply kses on your own. And we also have email templates so... probably not a big deal.

[dparker1005]

Closing in favor of #2000, which feels like a more intuitive approach