Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
In D8 everything can be commentable, but the node entity type is still very much hardcoded in comment_notify.
Proposed resolution
Generalize over entity types. The comment_notify table in its current form would need an additional "entity_type" column, and all occurrences of "node" and "nid" should be changed to "entity" (perhaps?) and "entity_id".
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#64 | comment_notify-2618182-63-64-interdiff.txt | 1.19 KB | gnuget |
#64 | comment_notify-2618182-64.patch | 68.5 KB | gnuget |
| |||
#55 | comment_notify-2618182-53-55-interdiff.txt | 1.96 KB | gnuget |
Comments
Comment #2
ashrafabedI am working on this issue as part of a class offered by http://debugacademy.com
Comment #3
ashrafabedI have the updated module functionality to work with all entity types. Will have a patch submitted next week.
Comment #4
ashrafabedI am attaching a patch for this issue.
Pieces missing from the patch before it can be tested/reviewed:
1- Schema change(s), update hooks
2- Creating a separate e-mail template for non-node notifications (due to token support for [node], but no token support for [entity])
3- Updating automated tests
Items 1 & 2 will be completed by a class at http://debugacademy.com within about 24 hours.
Comment #5
ashrafabedI had attached the wrong patch. Updating file attachment.
Comment #6
ashrafabedThe full patch is attached (but automated tests have not been updated). Could this can be merged in as-is? Then automated tests can come next, as well as potentially another e-mail template as part of a separate issue
Comment #7
peezy CreditAttribution: peezy as a volunteer commentedThanks for your work on this, @ashrafabed. I successfully applied the patch to the latest dev, but I'm getting this PHP error every time I comment on a user:
Also, I am unable to create users as described here: https://www.drupal.org/node/2684617
Is there a chance that the patch needs to get re-rolled against the latest dev? Also, I'm using PHP 7 and D8.3.7 in case that makes a difference.
Comment #8
ashrafabed@peezy I'll take a look at that shortly, thanks for letting me know
Comment #9
DamienMcKennaThis seems great but there are a few things that need work:
Comment #10
DamienMcKennaSome fixes.
Comment #11
DamienMcKennaEven with all this the emails don't seem to actually get generated.. Still working on it.
Comment #12
DamienMcKennaI think the custom table needs to be expanded to track the entity type and we need separate notification messages for each entity type.
Comment #13
DamienMcKennaNo, the custom table doesn't need to track the entity type but there definitely needs to be separate notification messages for each entity type.
Comment #14
DamienMcKennaI had forgotten to handle the previous interdiff properly after the last patch, so this interdiff is against #6.
This appears to be working for both nodes, users, etc.
Still need to work on notification messages.
Comment #15
DamienMcKennaComment #17
DamienMcKennaThis now allows editing different messages for each entity type that is supported.
Comment #18
DamienMcKennaI wonder if it would be better to move some of these settings to the comment field settings?
Comment #20
DamienMcKennaMixed up the schema.yml config. This should work.
Comment #22
DamienMcKennaFixed the constant being renamed.
Comment #24
DamienMcKennaA small UX improvement for the settings page - it automatically hides the email fields for an entity type if none of the comment types for that entity type are hidden.
Comment #26
DamienMcKennaThis should fix errors.
Comment #27
DamienMcKennaTests are green. Sweet. Now to see if someone would be so kind as to test it out.
Comment #28
DamienMcKennaBTW there's a @todo item in the update script, the bundle configuration needs to be updated to match the new structure.
Comment #29
DamienMcKennaForgot to include the 'comment-subscribed' token type for the 'watcher' email.
Comment #30
DamienMcKennaThe patch results in the following error when a user registers a new account:
Comment #32
DamienMcKennaNever mind, some custom code was tripping it up.
Comment #33
DamienMcKennaThat said, there was a problem sending notifications on the user entity if the comment was generated during the user registration process as the $entity didn't have a getOwner() method. This adds an extra check to see if the $entity is a user object, and if it is it just uses that entity directly as the author.
Comment #34
DamienMcKennaForgot to use the full class namespace on the instanceOf call.
Comment #35
DamienMcKennaThis includes changes from #173979: Customized mail subject to make the subject lines editable, otherwise this error gets thrown when messages are sent: "undefined index: subject in _comment_notify_mailalert()"
Comment #36
toamit CreditAttribution: toamit commentedJust tested this patch on a custom entity and it works.
Thanks for sharing the patch.
Comment #37
DamienMcKennaRerolled.
Comment #38
DamienMcKennaThis includes a fix for #2925405: Notice in comment_notify_form_user_form_alter() function too.
Comment #39
ashrafabedI found two minor issues with the patch.
1. in src/Form/CommentNotifySettings.php, the visibility of the 'body' fields is determined using Drupal's #states, but the 'subject' field was not. So the subject field was always displaying, whether or not it should. That was fixed.
2. The subject label on the comment notify form does not say what entity type it is for. When multiple entity types are commentable, you couldn't tell what entity type you were creating the email notification template for. I added the entity type to the label: '#title' => $this->t('Default mail subject for sending out :entity_type notifications to commenters', [':entity_type' => $entity_type]),
Updated patch is attached.
Comment #41
ashrafabedThe second attachment in my previous comment should not have ended in .patch, so the tests that should have run actually did pass.
Comment #42
ashrafabedComment #43
toamit CreditAttribution: toamit commentedTested #39 patch against comment_notify-8.x-1.0-beta2 . I had to run update.php for this patch to work correctly
The following things were tested and worked
- Able to create comment notification option for non-node custom entity
- Email notification are sent for authenticated subscribed users with All setting
Problem
- Email notification are not sent when Reply to my comments is subscribed.
There is another issue about this though at https://www.drupal.org/project/comment_notify/issues/2879914
Comment #44
ashrafabedThank you for the review @toamit. It is correct that running update.php (or drush updb) is necessary to apply the patch.
The problem you mentioned is in fact unrelated to this issue, and fits in the other issue you linked to.
I'm using this patch on production today, it is working very well for our comment-heavy website. If you've reviewed the work and feel comfortable switching from needs review to Reviewed & tested, feel free to - I'm looking forward to this (hopefully) getting merged into the project.
Comment #45
toamit CreditAttribution: toamit commentedBased on functional test, updating the status to RTBC. No code review was performed by me, if needed please revert back the status and assign it to a code reviewer.
Comment #46
Silvan Wakker CreditAttribution: Silvan Wakker commentedThe latest patch (from #39) failed applying against the latest version of dev (
SHA 01751b118ce3765fe3af07a36ca8c6f6687c6534
). I've tried to manually apply all the rejects and the module seems to be working fine but I can't figure one thing out. In the filecomment_notify/src/Form/CommentNotifySettings.php
at line 80 (after applying patch), a variable $type is called which is no longer created because of a deletion in patch #39 at line 792 (in the patch file). Not too deep into this issue yet, hoping someone can give a pointer to what to do with this.Comment #48
nevergone CreditAttribution: nevergone commentedThe patch apply cleanly.
Comment #49
ashrafabed@Silvan Wakker good catch with the $type variable:
$anonymous_problems[] = $type->link($type->label());
Like you said, the $type variable is no longer created, so that code would fail. It needs to be updated - I plan to address it in the next few days.
Comment #50
gnugetI addressed #46 at #2938134: Invalid placeholder (!types) in string
Comment #51
gnugetA rebase of #46.
Comment #53
gnugetOk.
This should fix the tests and a small bug that I found.
We need to finish the
@todo
added in thecomment_notify_update_8100
function before to commit this because there are users that are using this with multiple comment fields (#3020673: False positive notifications with multiple comment types per node).This looks great! thanks all for all work so far, I will continue working on this so we can finally commit it.
(At this point there is not even an RC version, are we sure that we want to pump this until the 8.1 version?)
Comment #54
gnugetComment #55
gnugetOk, In this patch I fixed the @todo in the hook_install.
I found a small bug but I'm going to work on the weekend.
Comment #56
gnugetThe bug that I found was basically #46 (again) I fixed it at #2938134: Invalid placeholder (!types) in string bu the code was refactored on this patch and the problem returned.
So this time I fixed it and wrote a test so we can be sure that this is won't come back again.
I think the only thing missing is a test testing a comment notification made in a non-node entity type.
I will work on that next week.
Comment #58
gnugetUps, small bug.
It should be fixed in this patch.
Comment #59
gnugetI wrote a test with a different entity type.
And this makes me realized that this will have some limitations, for instance the entity_terms haven't
uid
field, and the uid field of the users is the user id and not the user who creates the other user, this will break theSubscribe users to their entity follow-up notification emails by default
option for these entity types.I think we can create a follow-up where we add a message letting the the user know about this.
Comment #60
ashrafabed@gnuget glad to see progress being made here!
I encountered that issue with uid as well (because I use this module on a custom entity type w/out uid).
The solution I was planning on is potentially providing a hook which other modules can use to replace uid with whatever is appropriate for their entity type. That's most likely out of scope here, but thought it might be worth mentioning as a possibility.
Comment #61
gnugetHi ashrafabed,
OK, sounds interesting that solution. Let's discuss it in a follow-up, thanks!
I will wait a couple days in case someone else wants to test my patch before to commit it. And if nothing went wrong I will do a new release next week.
Comment #62
Silvan Wakker CreditAttribution: Silvan Wakker commentedI'm testing this now and it does not seem to work all the times. I'm trying the following scenario's (on a custom entity):
1. Post a comment as an anonymous user (Bob), as the author I'm not getting an e-mail
2. Post a comment as author, Bob gets an email as he configured
3. Reply as the author to Bob, an email is sent to Bob
4. As Bob, post a new comment. Author is now receiving an email, but that is because he posted a comment and has configured to receive updates about all comments
Note: the default setting are "Notify me when new comments are posted" checked and "All comments". I have not touched these in any scenario.
It seems that the author of a piece of content is not receiving the author emails, the ones configured under: "Default mail subject for sending out custom_entity notifications to authors".
After typing that last sentence I realised you need to configure this setting on the user page. I'm going to leave this here for other people that might not have read INSTALL.txt and are confused. Go to user/{id}/edit and tick the box "Receive content follow-up notification e-mails".
I've performed a code review and fixed some code style violations.
P.s. a suggestion for #60: don't use hooks, use the Symfony EventDispatcher/Subscriber.
Comment #63
gnugetThanks a lot Silvan Wakker!
Accord with this there still were 3 coding standard messages.
I just fixed them.
As soon as this be green I will commit the patch.
Thanks a lot!
Comment #64
gnugetOk, just one more.
Comment #66
gnuget