Great @joseph.olstad so would you suggest setting this RTBC or should we first further investigate the test failure?
Perhaps at least it might make sense to get this into dev?
Duplicate of https://www.drupal.org/project/media_embed_extra/issues/3499120 📌 Automated Drupal 11 compatibility fixes for media_embed_extra Active
Thanks, on it!
Thanks @florianmuellerch that was blazing fast!! :D 🚀
anybody → created an issue.
@Grevil: That's it!!!!!! CRAZY. Interesting to see this heavy side-effect...
Okay seems I'm close to the reason why it doesn't work:
\Drupal::config('user_registration_reminder.settings')->get("reminder_escalations.0")
works as expected and returns the German string at the same place, while
$escalationStepConfig = $this->getEscalationStepConfig($escalationStep);
doesn't and returns the English string.
So I pretty much guess this is another reason why we shouldn't use this convenient anti-pattern:
$this->config = $configFactory->get('user_registration_reminder.settings');
Done, please review: https://localize.drupal.org/translate/languages/de/translate?project=use...
Okay I think separate issues should be opened for these upstream fixes. Quite self-explaining ones. So we have the discussion elsewhere.
@Grevil & @LRWebks: Could you please ensure all fixes here are also reported upstream (if the code was copied)? Please open a MR for that in a separate issue, if it has the same bugs.
Thank you!
NOTE @anybody. For some reason there is NO hook_mail() implementation anymore for 11.x !!!
The reason is:
https://www.drupal.org/node/3496786 →
Thank you, I'll check why it doesn't work. Code looks correct to me, now we know that core does it the same way!
Okay let's leave this open for further discussions and possible implementations in the future. Thank you @Grevil!
@grevil: As long as you're "in" the module, could you write a short list, which tests might make sense here and how?
I think we'd need to mock the SDK, that should make it easy to track most of the functionality. I think this would be a great learning for us. Still minor.
Assigned to @grevil to ping me, once we're ready with all the other stuff.
@grevil please contact Posthog support and ask for best practices. Also ask how to identify an anonymous user server-side (null value) - if it's not possible to identify them. Frontend JS allows that, which is a bit strange.
All green and working fine now!
I'll do it. Not many...
@Grevil: Any idea how to get the translated config per user?
// @todo: We need to get the config in the users language!
$escalationStepConfig = $this->getEscalationStepConfig($escalationStep);
Currently, the mails are always sent in one language...
I was searching for a solution for half an hour without valid results. This MUST be possible? xD
anybody → changed the visibility of the branch 3501495-write-tests to hidden.
Thank you @avpaderno - I'd just like to fix things and push it forward.
@bkosborne thanks for your feedback! So what would you say, how far are we away from RTBC?
The tests are green, so if the tests are fine, it should be fine too, but yeah this so complex, so I'm still afraid to merge it, even if I tested it manually.
Works great!
@kushagra.goyal could you please add another test for that in MR!31? Thanks!
@bkosborne which other tests might be helpful to ensure we don't break something in this complexity?
@smustgrave I think so. We should tackle this in modules like Klaro oder COOKiES!
Nice, thanks @Grevil!
Can we reproduce this with any other form?
Looking for possible reasons, it seems to me that maybe the "host" key in the form is a problem for a contrib module?
We should check with a different key name first.
anybody → created an issue. See original summary → .
joachim namyslo → credited anybody → .
Thanks @jurgenhaas!
What exactly do you mean by:
let ECA base fields for entities.
I guess the sentence is broken, or I'm reading it wrong?
Maybe you mean allow ECA reading base field values? Or add base fields dynamically?
(I guess first already works and second would indeed be super powerful!)
Guess that belongs into a separate issue then?
I would have definitely used ECA for a simpler approach, but the multi-step process was requested by our client and that's not a simple one.
So I'll start with more simple processes in ECA :)
One last question regarding the way ECA works in this context:
Would it be possible (and correct) to start a "long running" process with a user registration, which ends if a users logs in or with a deletion of the user after X days, after one reminder email?
I'd just like to understand, if ECA is typically designed for short actions like rules or also for user-focused long running (and maybe even never ending) processes?
Thank you - and after that let's come back to the core of this request :D
Thanks @jurgenhaas - that totally makes sense, but we don't want a field for this technical metadata, if possible. Sadly, this is a pure technical impediment from Drupal history (having user.data as serialized array... not available to view).
So as we need to get this thing up and running, I decided to create a module instead of using ECA for now: https://www.drupal.org/project/user_registration_reminder →
Of course, I'd be super happy to list ECA and a receipt as alternative on the module page one day.
But I think for the moment it would have cost me a lot more time to dive deep enough to solve this good enough in ECA.
But hey, at least we created our first ECA action here recently: ✨ Provide ECA integration Active Hoorray 🎉
We'll keep on warming-up with ECA. Thank you so much, once again, for the impressive ECA module and your even more impressive and friendly support!
I'd like to keep this open, hoping for someone more experienced with ECA (or coming back here one day perhaps) to create a receipt for this typical registration use-case.
Let's do this later. Definitely useful, but for example we'll have to mock the SDK.
Nice! I left some final comments. Also tests are failing here.
Thanks @poker10 - I wasn't aware of that module, so for now I added it as alternative to the module page: https://www.drupal.org/project/watchdog_mailer → Would be great the other way around also.
If you or someone else could make a short comparison, I'd be super happy to give it a try.
I'll have a look at logging_alerts module as soon as I have the time!
Thanks @quietone! You're right, and I think we could even change this into a bug report as it's unexpected behaviour?
Still, I think we can make it minor, as nobody else seems to have run into this and we finally skipped this approach entirely. Still I think the issue exists and is worth to be kept open, if someone else runs into this niche case?
No I don't think it's a duplicate of that issue, this is about the event not being called at all (while expected) and the other one is about the URL.
Thanks for taking the time to read this! :)
Nice, just add the https://posthog.com/docs/configure/initialization-configuration link to section in the README also for details. Then this is fine for me!
Hi @jurgenhaas and thank you for the rapid response!
I also thought about this and indeed there are tricky ways to to this stateless, but they also become risky. I'm not sure yet, what's more kind of "the ECA way" for per-user-states or using user.data store service for a solid stateful implementation.
If only sending one reminder, this is tricky enough stateless, but stateless logic becomes even trickier (and harder to understand and maintain for the site owner) if there's a defined process of emails being sent out, until the final reminder is reached or the user verifies his account / logs in.
This escalation process would at least be easier to understand and maybe is more robust using a stateful approach. The user might get flagged with a something like an "escalation step" + date user.data variable, so we wouldn't have to mess around with special cases like older accounts and non-running cron.
But I have to admit, I have no idea yet, what this means for ECA and if a by-user-process would be a good idea, that might start with the registration, maybe.
If you had this requirement, how would you solve it in ECA?
(Sorry to ask this dumb question without providing much input on the solution myself yet, but I'm using ECA for the first time and am totally unaware of all the details and best practices)
Give it a try in a MR and we can have a look.
anybody → created an issue.
Thank you so much!!
@oksana-c could you please tag a new D11 compatible release with priority?
@heine could you prepare a MR for that?
Merged, to get this done.
Happy to merge this, once it has tests and is reviewed by experienced members of the community. Won't have the time to review this myself in the near future.
@megakeegman totally agree ,could you create a MR please?
Done!
Nice, thank you @jfeltkamp! :) Looking forward to that! Should you need assistance, feel free to ping us.
Needs work on the texts.
@adamps could you maybe tell if my assumption is correct or add an example?
We could just override the #maxlength from core here: https://git.drupalcode.org/project/layout_options/-/blob/8.x-1.x/src/Opt...
If we don't want to wait for the core fix.
There's no limit implemented. Once again it's just inherited from Drupal Core's (not helpful) limit: ✨ Increase or remove default textfield #maxlength=128 Active