backdrop-contrib / simplenews

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

Incompatibility with DB prefixes #18

Closed djzwerg closed 1 year ago

djzwerg commented 1 year ago

Today I've seen this bug which breaks my work on a new site: After enabling simplenews I wanted to make some changes to another taxonomy term. While clicking on /admin/structure/[tid]/edit or /admin/structure/[tid]/delete the following error notice was shown:

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'web_tid' in 'where clause': SELECT tid as min_id FROM {simplenews_category} WHERE {tid} = 15; Array ( )

BD log says:

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'web_tid' in 'where clause': SELECT tid as min_id FROM {simplenews_category} WHERE {tid} = 16; Array ( ) in simplenews_form_alter() (line 780 of /mypath/modules/simplenews/simplenews.module).

The tid '15' is not a simplenews category and was created before enabling simplenews. To make my changes I have to disable simplenews module and re-enable it after that (without uninstall). But I think this should not be the way to edit taxonomy terms while simplenews module is used.

olafgrabienski commented 1 year ago

While clicking on /admin/structure/[tid]/edit or /admin/structure/[tid]/delete [...]

I'm trying to reproduce the error and am wondering about the paths. On my site, they don't contain admin/structure but taxonomy/term, e.g. taxonomy/term/[tid]/edit.

olafgrabienski commented 1 year ago

Hm, at first sight, I'm not able to reproduce the issue. These were my steps on a fresh Backdrop demo site:

I was able to edit and then delete the term and didn't see any error. Am I missing something?

alanmels commented 1 year ago

@djzwerg, if you analyze the code-block that starts from https://github.com/backdrop-contrib/simplenews/blob/745e535c154cc27eae0b771da08fd44842c007d1/simplenews.module#L772 it looks like this:

if ($form_id == 'taxonomy_form_term') {
// Prevent creating taxonomy terms and instead redirect to Newsletter configuration page to add categories.
  if (strpos($form['#action'], '/admin/structure/taxonomy/newsletter/add') !== false) {
      backdrop_goto('admin/config/services/simplenews/add');
    }
  }
  if (arg(0) == 'taxonomy' && is_numeric(arg(2))) {
    $tid = arg(2);
    $found = db_query("SELECT tid as min_id FROM {simplenews_category} WHERE {tid} = $tid")->fetchField();
    if (!empty($found)) {
      if (arg(3) == 'edit') {
        // For some reason backdrop_goto($path) is not working;
        $url = $base_url . '/admin/config/services/simplenews/categories/' . $tid . '/edit';
        header('Location: ' . $url);
      }
      if ($form_id == 'taxonomy_term_confirm_delete') {
        $url = $base_url . '/admin/config/services/simplenews/categories/' . $tid . '/delete';
        header('Location: ' . $url);
      }
    }
  }

which means the redirection from taxonomy edit/create/delete pages to newsletter edit/create/delete should kick in only when simplenews/newsletter assigned vocabularies or terms are involved.

So as @olafgrabienski explained without being able to reproduce the error on a fresh install, it would be tough to guess what's going on with your particular setup. If you could enable the Devel module and then place dpm('test') command in different places in the above-given block code to identify which redirection rule exactly is involved, would be really useful.

djzwerg commented 1 year ago

While clicking on /admin/structure/[tid]/edit or /admin/structure/[tid]/delete [...]

I'm trying to reproduce the error and am wondering about the paths. On my site, they don't contain admin/structure but taxonomy/term, e.g. taxonomy/term/[tid]/edit.

@olafgrabienski to clarify on my site the path is e.g. /admin/structure/taxonomy/newsletter and on this view e.g. /taxonomy/term/[tid]/delete. Sorry for that.

djzwerg commented 1 year ago

@alanmels I've tried this:

if ($form_id == 'taxonomy_form_term') {
      dpm('line 772');
    // Prevent creating taxonomy terms and instead redirect to Newsletter configuration page to add categories.
    if (strpos($form['#action'], '/admin/structure/taxonomy/newsletter/add') !== false) {
        dpm('line 774');
      backdrop_goto('admin/config/services/simplenews/add');
    }
  }
  if (arg(0) == 'taxonomy' && is_numeric(arg(2))) {
      dpm('line 778');
    $tid = arg(2);
    dpm('line 779');
    $found = db_query("SELECT tid as min_id FROM {simplenews_category} WHERE {tid} = $tid")->fetchField();
    dpm('line 780');
    if (!empty($found)) {
        dpm('line 781');
      if (arg(3) == 'edit') {
        dpm('line 782');  
        // For some reason backdrop_goto($path) is not working;
        dpm('line 783');
        $url = $base_url . '/admin/config/services/simplenews/categories/' . $tid . '/edit';
        dpm('line 784');
        header('Location: ' . $url);
        dpm('line 785');
      }
      if ($form_id == 'taxonomy_term_confirm_delete') {
        dpm('line 787');
        $url = $base_url . '/admin/config/services/simplenews/categories/' . $tid . '/delete';
        dpm('line 788');
        header('Location: ' . $url);
        dpm('line 789');
      }
    }
  }

And BD shows this:

line 778 line 779

Is that helpful?

djzwerg commented 1 year ago

@olafgrabienski can please try to on your install what happens if you add a term on your simplenews vocabulary manually (via /admin/structure/taxonomy/newsletter and not via /admin/config/services/simplenews/add) and after that deleting it manually?

djzwerg commented 1 year ago

I've tried to uninstall simplenews, deleting the vocabulary and re-install but that won't change anything.

djzwerg commented 1 year ago

Note: web_ is my db prefix. If I delete the prefix in my db and settings.php it works. So it looks like this issue is caused by using a db prefix?!

djzwerg commented 1 year ago

On another site I was able to reproduce this issue using a db prefix. @alanmels maybe it would be helpful to add a hint to the readme file?

alanmels commented 1 year ago

And BD shows this:

line 778
line 779

Is that helpful?

@djzwerg, actually it is. Because, if you read the code those two lines should trigger for ALL vocabulary terms, but the next line is important:

$found = db_query("SELECT tid as min_id FROM {simplenews_category} WHERE {tid} = $tid")->fetchField();

as it takes the current term in URL and checks if it is a simplenews term or not, and it performs redirections only if it is

    if (!empty($found)) {

So since dpm() didn't trigger anywhere else, it means no redirections should have been involved.

alanmels commented 1 year ago

Note: web_ is my db prefix. If I delete the prefix in my db and settings.php it works. So it looks like this issue is caused by using a db prefix?!

That's interesting. I need to run some tests.

alanmels commented 1 year ago

@djzwerg can you please check your MySQL or MariaDB version?

djzwerg commented 1 year ago

@alanmels of course:

alanmels commented 1 year ago

@djzwerg, could you please make changes in couples places as shown on https://github.com/backdrop-contrib/simplenews/compare/1.x-1.x...alanmels-patch-1 and see if that makes the issue be gone?

djzwerg commented 1 year ago

Yes that fixes it. Thanks!

olafgrabienski commented 1 year ago

can please try to on your install what happens if you add a term on your simplenews vocabulary manually (...) and after that deleting it manually?

@djzwerg As the issue is fixed, I guess further experiments aren't necessary here. Let me know, if you're however interested.

djzwerg commented 1 year ago

can please try to on your install what happens if you add a term on your simplenews vocabulary manually (...) and after that deleting it manually?

@djzwerg As the issue is fixed, I guess further experiments aren't necessary here. Let me know, if you're however interested.

@olafgrabienski you're right.