search

backdrop-contrib / simplenews

BackdropCMS port of the Simplenews module for Drupal 7
GNU General Public License v2.0
1 stars 3 forks source link

Can't send newsletter #15

Closed olafgrabienski closed 7 months ago

olafgrabienski commented 1 year ago

On a site upgraded from Drupal 7, I'm not able to send a Simplenews newsletter. After hitting the Send button, I get the following messages instead, depending on the send action (test vs. real):

Send one test newsletter to the test address:

Send newsletter:

olafgrabienski commented 1 year ago

I hope the Simplenews history of the specific site is helpful to debug the issue:

alanmels commented 1 year ago

Thanks for the insights.

First of all, comparing the line numbers I see you're not on the latest dev. For example, the getLanguage() function is not at line 570 of /.../modules/simplenews/includes/simplenews.source.inc currently. So to be on the same page please install the latest dev instead of patching.

It's true there are lots of language/langcode naming mixups, and because just replacing them all at once won't work, we have to run on and get fixed each of them individually. The last one, for example (see the change on https://github.com/backdrop-contrib/simplenews/commit/aad300e6cce7f005676f0760db9d484f3b5ff121) since I could not replicate the issue on my local setup, it was a rather blind attempt to fix #10. According to that change the return $this->getSubscriber()->langcode; was changed to return $this->getSubscriber()->language; so if you were using the last dev then your setup should not complain about:

Notice: Undefined property: stdClass::$langcode in SimplenewsSourceNode->getLanguage() (line 570 of /.../modules/simplenews/includes/simplenews.source.inc).

So while changing instances of langcode to language is easy and can be performed blindly just by trusting users reports for specific lines, it's completely different story for more complex issues as:

    Notice: Trying to get property 'tid' of non-object in SimplenewsSourceNode->__construct() (line 388 of /.../modules/simplenews/includes/simplenews.source.inc).
    Error: Class name must be a valid object or a string in SimplenewsSourceNode->initCache() (line 440 of /.../modules/simplenews/includes/simplenews.source.inc).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'tid' cannot be null: INSERT INTO {simplenews_newsletter} (nid, tid, status) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 15863 [:db_insert_placeholder_1] => [:db_insert_placeholder_2] => 1 ) in simplenews_newsletter_save() (line 1965 of /.../modules/simplenews/simplenews.module).

I'm afraid it's not possible to troubleshoot those without being able to replicate them on my local setup. So you would have to either track the cause down and offer PR to merge or send me your current database, so I could run tests. I only need relevant part that would help me to replicate the issues on my end.

olafgrabienski commented 1 year ago

Hey Alan, thanks for your reply! You're right, the site wasn't up to date, I hadn't seen the new release from a few days ago.

In the meantime I've updated to version 1.x-1.0.5, and indeed the first (langcode related) notice is gone. Regarding the other messages I'll try to investigate further. Unfortunately, it's a fairly complex site, and I'm missing the necessary coding skills, but I'll find someone to help me.

olafgrabienski commented 1 year ago

One additional note: Now, when I try to add a new Simplenews node, it's not possible to save the form, and I get the following error message at top of the form:

No newsletter category field is configured. Check ... @todo

For reference: https://github.com/backdrop-contrib/simplenews/blob/6b8bb379b4a874e9700e7818505dc11a84029862/simplenews.module#L378.

Edit: forgot to mention: The field field_simplenews_term is however present in the content type, and it has a value in the node I'm trying to add.

alanmels commented 1 year ago

Tough to guess what's going on there, so could you please:

If a fresh install works a expected, but persists on the project you are working on only, then please compare what's different in settings between the fresh test setup and your current project. Check the content type settings, make sure it is configured properly for Simplenews.

olafgrabienski commented 1 year ago

Yes, it's really tough to guess what's going on. I've however installed a fresh Backdrop site to verify that the problem doesn't exist there. That's what I expected.

Comparing all differences between the vanilla site and my project isn't possible, as the latter is both an upgrade from D7 and quite complex. I guess something went wrong during the upgrade, leading somehow to the current issue. So, I'm considering to make a test upgrade of a fresh D7 site with Simplenews for comparison. (Alan, have you ever done this?)

That said, I've noticed another thing which may also help to narrow down the reason for the issue: When I add a new content type and tick the "Use as simplenews newsletter" option, I get the following message, and the Newsletter category field (field_simplenews_term) is not added to the content type.

FieldException: Attempt to validate a field with no field name specified. in field_validate_field() (line 237 of /.../core/modules/field/field.crud.inc).

Somehow the Newsletter category field isn't recognized as such. I've also had a look at the correspondent Newsletter vocabulary on my site, but didn't find any strange settings there.

alanmels commented 1 year ago

From what you told something might be wrong with taxonomy term. Make sure there is a respective taxonomy for the newsletter. You can create a new newsletter and compare how it corresponds with respective taxonomy term and compare with your existing/old newsletter.

I did many Drupal 7 to Backdrop migrations, but none with Simplenews. Actually, one project had Simplenews, but I never tried to deal with Simplenews settings/comparison, because it was much easier just to configure new content type of Simplenews type and start using newsletters from fresh.

I am afraid while porting I never cared of upgrade path from Drupal 7 as my main goal was to get the module work for Backdrop, so that part probably needs further development. Unfortunately, as I said before it is not easy to guess remotely what kind of issues you are hitting. I don't mind taking a look if you can give me a temporary access. Otherwise, I'd recommend you to just start using a new content type as Simplenews.

Feel free to report any issues if you can replicate on fresh installs. As for customized/migrated projects, unfortunately too many things could be effecting and won't be possible for me to help remotely.

olafgrabienski commented 1 year ago

Thanks for your reply, I agree it's hard to guess anything in this situation. Thanks also for the hint to create a new newsletter. Are you suggesting to create a new Newsletter taxonomy term or to create a new Simplenews content type?

olafgrabienski commented 1 year ago

Okay, I've finally made a test upgrade of a fresh D7 site with Simplenews to Backdrop, and I noted a couple of Simplenews problems after the upgrade. Not the same errors as in the original description of this issue, but some issues might be related.

I think it makes however sense to file a new report regarding the upgrade from D7. Here it is: https://github.com/backdrop-contrib/simplenews/issues/16

olafgrabienski commented 1 year ago

In the meantime, I'm able to send a newsletter, not being sure which setting lead to this success, and I still get many Simplenews related notices on the site. While I'd like to not close this issue immediately, I'd suggest to focus on #16 for the time being.

alanmels commented 7 months ago

I believe most if not all of the errors and notices have been addressed since this issue was created, and therefore I'm closing this. If they still persist for you, feel free to report each error or notice individually on its own issue page.