Pass the email instance as parameter to substituteMarkersHook()

[cundd]

I think it would be handy to pass the email instance to the substitute markers hook, so one can retrieve the email data through the object and get access to the underlying newsletter instance.

I hope I didn't screw up the code style this time. But I only added one word :wink:

Thank you for this extension!

[PowerKiKi]

Do you actually use any of Newsletter hooks ? I never heard of someone who does. TBH I was wondering if it's something that could be removed/heavily refactored. So I am looking for feedback on that.

Also I am not too keen on passing an instance of our model, because it would introduce a tight coupling between our code and the hook implementation. From what I understand from your description passing $email->getRecipientData() would be enough, isn't it ?

[cundd]

To describe what I'm doing: I am implementing a system which allows users to access data X times max. Our use case is to allow the receivers of the email to show a sent news entry max 5 times. What I need is a way to modify the links in the emails and append some parameters. These parameters are different for each receiver.

Is there a better way to do that/post-process the sent data?

I just thought it would be nice to get access to the models. Especially to know which newsletter is currently processed. If you think it's a bad design decision, feel free to close the PR.

Thank you for having a look and the discussion.

[PowerKiKi]

My first thought would be to use a SQL recipient list so you have a lot of control on recipient data. From the manual:

SQL recipient lists are, by far, the most flexible and powerful way do define a list of recipients. It allows the dynamic composition of strings that can be used in newsletter content. And it also allows you to take action (SQL queries) upon specific events (bounced email, unsubscribe). Thus we strongly recommend the use of SQL Recipient List.

Any valid SQL is valid (we do only very minimal treatment on that string), so it allows you to generate custom string to be used as markers for each recipient based on potentially very complexes SQL queries (think JOIN, subqueries, VIEW, etc). I'd imagine something like this with made up tables:

SELECT email, CONCAT('http://example.com/?param=',  fe_user_acess_history.access_count) AS my_custom_link_marker
FROM fe_user INNER JOIN fe_user_access_history ON (fe_user.uid = fe_user_acess_history.user)
WHERE NOT deleted;

Then in your Newsletter you'd have someting like:

<a href="http://my_custom_link_marker">check it out</a>

Could this solve your use-case without using the hook ?

[cundd]

I still like the idea of a PHP based solution. I need to create a random authentication code for every resource-user combination to keep track of who accessed what.

Would Signals and Slots be a more appealing solution for you?

[PowerKiKi]

Indeed signal/slot seems to be fashionable nowadays, but I won't have time (nor interest TBH) to implement that in short term.

So I suggest the quick-and-dirty solution. Would $email->getRecipientData() be enough to cover your use-case ? or do you actually need data form $newsletter too ? and if so what exactly would you need ?

[cundd]

The email UID would help, or something else that's unique.

[cundd]

What I also like about passing a concrete class: One gets the benefits of method hints, so that I don't have to var_dump the markers array to see whats really in there. And I am sure how I can get the recipient address (in case it can be another field than $markers['email']). You see, I'm a fan of stricter typing :wink:

[PowerKiKi]

I totally agree on stricter typing. Maybe we can reach a compromise. What about introducing a minimal interface that covers your need ? that would allow you to implement whatever you need, while still giving me the freedom to modify most of the model without having to worry too much about breaking hooks for people out there.

Of course you technically could still type hint against the implementation and use all methods, but then that's your responsibility, not mine.

From what you said so far, I'd suggest something like:

namespace Ecodev\Newsletter\Domain\Model;

interface EmailInterface {
    public function getUid();
    public function getRecipientData();
    public function getRecipientAddress();
}

What do you think ?

[cundd]

I think that's a great idea! And the shown interface would provide what I need!

Thank you very much!

[PowerKiKi]

Could you please experiment with develop branch and let me know your feedback ?

[cundd]

Works great for me.

I'm still calling getNewsletter() on the email instance - but as you said, if that crashes it's my own fault. :fire:

I'm closing this pull request!?