I'm pretty sure all the date functions this module uses are defined in Date API rather than date module, since the latter is merely for providing CCK fields.

I've checked a few of the ones that start 'date_' and they're all in Date API.

This would make testing easier, and potentially save an installed module if the user doesn't need date fields.

CommentFileSizeAuthor
#11 without-todo-1372140.patch1.38 KBberdir
#9 change_date_dependency.patch1.65 KBberdir
#1 1372140.simplenews_scheduler.date-api-dependency.patch457 bytesjoachim

Comments

joachim’s picture

joachim commented
Status: Active » Needs review
StatusFileSize
new 1372140.simplenews_scheduler.date-api-dependency.patch457 bytes

Here's a patch.

dgtlmoon’s picture

dgtlmoon commented

this may be a left over from the drupal-5 days, i think it was initially trying to use the date calls to figure out local timezone to send on

joachim’s picture

joachim commented

Do you mean that it's ok to make this change?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Seems to be fine for me.

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Fixed

Thanks!

- #1372140 by joachim: Fixed dependency on date module to be date API instead.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

joachim’s picture

joachim commented
Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Closed (fixed) » Patch (to be ported)

This needs forward-porting to D7.

joachim’s picture

joachim commented

The module dependencies declared in our tests will need changing too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new change_date_dependency.patch1.65 KB

Let's ask testbot what he thinks about this, changed dependency.

I also simplified (D7 takes care of our dependencies automatically) and fixed (always use array_merge() with numeric keys or we lose array entries) setUp() and documented it according to the standards.

joachim’s picture

joachim commented
Status: Needs review » Needs work 
+++ b/tests/simplenews_scheduler.test
@@ -13,14 +13,22 @@
+ *
+ * @todo Check if it is possible provide the needed variables in setUp() and
+ * revert them in tearDown().
  */

Is this todo related to this patch? It doesn't seem like it to me.

I'm happy to add it to the code anyway, but maybe as a separate commit?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new without-todo-1372140.patch1.38 KB

Hah, you *are* picky about patch context ;) We can just as well open an issue to look into that, an issue to add a todo to fix something is kinda weird ;)

Here is a patch without it.

joachim’s picture

joachim commented
Status: Needs review » Fixed

> Hah, you *are* picky about patch context ;)

Lol... It's all about saving the kittens! :D

- #1372140 by Berdir: Fixed module dependency to be on date API, rather than date module.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.