As I develop a new newsletter and test sending it using scheduler, I end up creating many newsletters. Then, I go in and delete both the editions and the parent. Sometimes, I delete the parent first, then the editions.

Anyway, now looking at the DB tables, I see rows in simplenews_scheduler_editions that are orphans, referring to nodes that no longer exist. So, I manually delete them from the raw tables.

I realize in production, you wouldn't want to delete editions since there are emails out there that link to the published editions, but for testing, I just want to clean out all the testing junk.

Again, I'm new to Drupal and PHP, so if someone could just point me in the right direction, function, file name, I can attempt to dig into this.

CommentFileSizeAuthor
#9 1795674-9-editions-orphaned.patch612 bytesjking1224
#7 1795674-tidy-editionstable.patch488 bytesdgtlmoon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented

Coming new to PHP and Drupal, I don't know the code very well, or syntax.

But looking at this issue, it seems the problem lies in file: simplenews_scheduler.module
function: simplenews_scheduler_node_delete()

It looks like here is where delete from simplenews_scheduler occurs and so should also delete simplenews_scheduler_editions as well for pid = nodeid? or eid = nodeid? or both?

code something like:
db_delete('simplenews_scheduler_editions')
->condition('pid', $node->nid)
->execute();

or
db_delete('simplenews_scheduler_editions')
->condition('eid', $node->nid)
->execute();

or both?
is that right?

This is to match up with the insert that occurs in function _simplenews_scheduler_new_edition(): ...db_insert('simplenews_scheduler_editions')...

There is also what appears to be a "watchdog" entry created during the new_edition(). Do those have to be deleted? its looks more like a log entry.

joachim’s picture

joachim CreditAttribution: joachim commented

Off the top of my head I can't remember what the pid and eid mean -- probably parent and edition ids. Check hook_schema() which should have descriptions of those (and if not, it's a bug!).

Watchdog entries don't need to be deleted -- they are cleaned up by the system periodically.

joachim’s picture

joachim CreditAttribution: joachim commented 
      'eid' => array(
        'description' => 'The node id for the edition.',
        'type' => 'int',
        'not null' => TRUE,
        'default' => 0,
      ),
      'pid' => array(
        'description' => 'The node id for the parent scheduled newsletter node.',
        'type' => 'int',
        'not null' => TRUE,
        'default' => 0,
      ),

So it's the eid that gives you the node that is directly tied to this record.

Hence you need to delete based on the eid.

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon commented

Is this a bug or support request? are you deleting the node via node/23/edit or just editing the table directly?

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented

This is a bug report. Rows in simplenews_scheduler_editions get orphaned when you delete a newsletter. They should rightfully be deleted when their related newsletter is deleted.

I am deleting newsletters via "Edit" newsletter, then "Delete". I have to manually remove rows from simplenews_scheduler_editions via SQL or phpMyAdmin.

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon commented

yeah, i see we just have

/**
 * Implements hook_node_delete().
 */
function simplenews_scheduler_node_delete($node) {
  db_delete('simplenews_scheduler')
    ->condition('nid', $node->nid)
    ->execute();
}
dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon commented
FileSize
1795674-tidy-editionstable.patch488 bytes

So the problem is, we might want to keep the children/generated nodes, but clean up the editions table.

Patch is supplied, but will this cause more problems? Do we still need to know if a node is a generated edition?

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented

I think your patch is wrong. We are to be deleting the "sibling" records of the current node as found in simplenews_scheduler_editions.
As such the code should be:

db_delete('simplenews_scheduler_editions')
->condition('eid', $node->nid)
->execute();

And this answers the question of "do we still need to know if a node is a generated edition?" It doesn't matter. If we are deleting the node, it doesn't matter if its a generated edition or not, we should be also deleting the sibling row per above.

To my mind, its completely separate consideration as to whether:
a. we allow deleting a "template" newsletter node when generated editions exist
b. Do we cascade delete all generated editions when a template is deleted.
c. do we leave the generated editions behind (orphans?) when template is deleted.

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented
FileSize
1795674-9-editions-orphaned.patch612 bytes

I have now learned to create code patches. Here is code patch to delete sibling records of editions in table simplenews_scheduler_editions.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Active » Needs work

Looks fine.

A hook_update should also be created and executed:
We should join via simplenews_scheduler to simplenews_scheduler_editions and remove all orphans.

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented

It might be nit picky here, but I'd say that is a separate issue. My patch fixes the specific issue I was referring to in the original post: when delete a newsletter of either kind (parent or edition), the sibling row in scheduler_edition wasn't removed.

Your comment brings up a larger issue:
1. do we allow delete of a parent newsletter if editions exist?
2. If we allow to delete parent when editions exist, do we simply cascade the delete to all editions, do we warn that editions will be deleted and confirm or do we orphan the editions?

Do we warn that newsletter shouldn't be deleted if its been sent? Do we warn shouldn't delete parent newsletter if any editions have been sent? This is because a newsletter that has been sent contains links back to the site and those links will then be dead.

etc. All sorts of possible scenarios.

But in all the possible scenarios and possible choices made in code or by user, when delete a newsletter DOES occur, its sibling row in scheduler_edition should also be deleted regardless. hence, my patch.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

I didn't want to bring up a new issue and i don't agree.
I'm referring to the situation that due to your reported bug there are orphan records (from previous deleted editions).

If you fix an issue, you should always also cleanup the database, if possible.

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented

Ah, I misunderstood your point.

I believe your point is that this patch should delete all scheduler_editions records that exist, but do not belong to any currently defined simplenews node, regardless of whether they are parent or edition (i.e. currently existing orphans). These are rows that would have been created and orphaned by use the simplenews_scheduler module prior to the application of the patch.

Is this correct interpretation of your point? If so, I would agree that being very complete would require to do this. If so, its seems that this should be done at installation time of the patch (one time), not during the process of editing/deleting editions (many occurances over time). As I am new to Drupal, I don't know how or where to place code for such an event. It seems there is a place for installation/updating code.

If this is not correct interpretation of your point, then I am missing it.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Yes, it's just about providing a update hook.
Update hooks reside in the .install file and are only executed once on update of the module.

http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...

jking1224’s picture

jking1224 CreditAttribution: jking1224 commented

So, reading and learning.... looking at simplenews_scheduler.install file, it appears that I should create a new function in there: simplenews_scheduler_update_7003()

where I db_delete('simplenews_scheduler_editions') for any record in simplenews_scheduler_editions where
eid is not in node.

I don't know how to write that code, but I'll figure it out.

but is my function name, file name location and logic correct? Would I be doing this right?

dgtlmoon’s picture

dgtlmoon CreditAttribution: dgtlmoon commented

sounds good to me

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Warning: Related to #813948: make Edition Count available

I don't think we should delete by eid

->condition('eid', $node->nid)

There might be reason enough to persist simplenews_scheduler_editions till the related simplenews_scheduler record is obsolete. This would then make more sense:

->condition('pid', $node->nid)

The "right" design depends on the understanding of the concept.

joachim’s picture

joachim CreditAttribution: joachim commented

> The "right" design depends on the understanding of the concept.

I'd just like to chime in and say at this point I'm fairly confused and I'm going to need a strong cup of coffee before I review this one! If anyone wants to update the summary to give an overview of the tables and relationships involved, that would really help.