I just spent a very annoying and inexplicable bug I found on one of my pressflow sites in which update.php could not be able to be run.

What that has to do with this module you shall ask? Well, I have no clue, but after debugging I noticed that everything was messed up when the file of this module was load. After trying a few lines, I noticed that

define('SIMPLENEWS_COMMAND_SEND_NONE',4);

was the problem.

I am using simplenews 1.3 (and I can't change it) and that release has it already defined. Is it the 2.x version of this module only to be used with 2.x version of simplenews? Or it should work with either version?

If so, I would recommend a fix for this:

if (!defined('SIMPLENEWS_COMMAND_SEND_NONE')) {
  define('SIMPLENEWS_COMMAND_SEND_NONE',4);
}

I can submit a patch but it's pretty straightforward.

Comments

joachim’s picture

joachim commented
Title: SIMPLENEWS_COMMAND_SEND_NONE defined twice » SIMPLENEWS_COMMAND_SEND_NONE defined twice: incompatible with Simplenews 1.x?

So this is the situation:

1.x:

/**
 * NEWSLETTER SEND COMMAND
 */
define('SIMPLENEWS_COMMAND_SEND_NONE', 0);
define('SIMPLENEWS_COMMAND_SEND_NOW', 1);
define('SIMPLENEWS_COMMAND_SEND_TEST', 2);

2.x:

/**
 * NEWSLETTER SEND COMMAND
 */
define('SIMPLENEWS_COMMAND_SEND_TEST', 0);
define('SIMPLENEWS_COMMAND_SEND_NOW', 1);

and in simplenews scheduler module:

/**
 * NEWSLETTER SEND COMMAND
 *
 * Extends the options in Simplenews module.
 */
define('SIMPLENEWS_COMMAND_SEND_SCHEDULE', 3);
define('SIMPLENEWS_COMMAND_SEND_NONE', 4);

I am really not sure that we can work properly if we don't define that constant. For starters, we use it to create an option in the form.

> Is it the 2.x version of this module only to be used with 2.x version of simplenews? Or it should work with either version?

That may therefore well be the case. Would be best to get input from Simplenews maintainers on this too.

joachim’s picture

joachim commented
Status: Needs review » Active
hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

I am not saying not to define it, but define it if it doesn't exist. Other than that I don't see any conflict.

joachim’s picture

joachim commented

Right, but then I don't know what that will do to other parts of the code, such as:

    $form['simplenews']['send']['#options'][SIMPLENEWS_COMMAND_SEND_NONE] = t("Don't send now or stop sending");
hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

I have given an other thoughts, and maybe what you should do, which I don't think it's a bad practice after all, is to namespace your constants:

define('SIMPLENEWS_SCHEDULER_COMMAND_SEND_SCHEDULE', 3);
define('SIMPLENEWS_SCHEDULER_COMMAND_SEND_NONE', 4);

The worst case scenario here is that in simplenews 1.3x you will have maybe to None options, one from your module and one from simplenews, but even then, that's better that have it defined twice which is not really supported with PHP.

In simplenews 2.x you will still have the exact same functionality is now.

joachim’s picture

joachim commented

To be honest, I think support for Simplenews 1.x should probably be dropped from the 2.x branch of this module. We've various problems of which #1581808: Cannot manually create new users with simplenews scheduler active is one which I can't fix without possibly breaking for Simplenews 1.x, and working with two installs of Simplenews for both branches just isn't feasible.

joachim’s picture

joachim commented

There's also #1379284: simplenews_scheduler_form_alter() checks a nonexistent $node: reasons why we can't support 1.x are stacking up I'm afraid.

joachim’s picture

joachim commented

I've filed #1608300: [policy] Drop support for Simplenews 1.x in our 6.x-2.x branch to specifically discuss support for Simplenews 1.x.

joachim’s picture

joachim commented
Status: Active » Closed (won't fix)

The decision of the maintainers is that we shouldn't try to support both Simplenews branches in our one branch. A compelling case for this was the Simplenews maintainer saying this was a bad idea!

Accordingly, I'm closing this as won't fix.

If you want the features in the 2.x branch in 1.x, you're welcome to file patches, and I'll review them and commit them. Though I would suggest you upgrade Simplenews instead!