Posted by zilverdistel on March 15, 2011 at 7:19am
6 followers
| Project: | Scheduler |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
As promised in http://drupal.org/node/276376#comment-4207316,
and in a newly created issue as Eric kindly asked,
here's a patch for the scheduler-7.x-1.x branch that adds scheduled promote/unpromote support.
Please test/review it if you can.
| Attachment | Size |
|---|---|
| scheduler-with-promote.patch | 38.37 KB |
Comments
#1
#2
Rerolled patch (removed a call to dpm(), and changed all occurances of Demote to Unpromote).
#3
"Demote" is more proper English than "unpromote," which would actually never be included in a good dictionary because of the negating double prefix.
#4
And now D6? Probably the biggest difference is the query construction.
#5
Minor technicality: 2 space indentation on forms.
Concatenation - spaces around dot.
#6
Thanks for doing the patch, but my initial impression is that it is a huge amount of changes [5 files changed, 525 insertions(+), 57 deletions(-) it says at the top] for essentially a simple operation. The problem is that the pacth duplicates so much of the existing code with only minor changes. I think Eric may have been designing another way to do it, also including other actions to be done on the node. At minimum we should parameterise and re-use current functions, as this change as it stands adds a large overhead to those who work on this module maintain and maintain it.
We definitely should consider options to promote/demote but I don't think the method suggested here is the correct approach in terms of making it extensible for future enhancements.
Jonathan
(sorry to be negative, thank you for your patch and for getting the idea started).
#7
@NancyDru: Concerning "demote" instead of "unpromote". I choose for unpromote, because I saw it being used in core (I don't remember where, maybe in some docblock in code).
@Jonathan: You're absolutely right about the code duplications. I needed this asap for a client ... Also, I think as a first step it's actually very usefull to duplicate the code, because it shows where further abstraction is needed... Concerning other possible actions, in my opinion, to keep this module simple (for the user) we should only provide scheduling for the publish/promote/sticky options, since these options are in core. More exotic options could then be handled by the rules module.
#8
The patch is indeed quite "comprehensive" (scheduler.module: 1027 lines, patch: 910 lines). Since I do not trust large patches, I "merge" them manually anyway (copy&paste section by section) to see what's going on. That way I can also refactor the patch as I go.
I agree with zilverdistel about limiting the scheduled actions to those core options. Everything else can be done with rules.
#9
@zilverdistel: I'm not so picky about internal documentation, but anything that is visible to the end user should be in good enough English as to be translatable.
@Eric: I tried applying it manually to 6.x and got thoroughly lost about half way through. I may try it again because I need this within the next two weeks, if not sooner.
#10
I tried to use this and promote something, and this was the resulting error. There were no errors during patching.
#11
@Branndon: seems a db column is missing. Did you run update.php after patching?
#12
I did. I haven't worked on this site for a while now, but I'll report back if the issue hasn't been fixed yet. Thanks.