Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide simpler ScheduledTasks API #5301

Closed
tsteur opened this issue Jun 5, 2014 · 8 comments
Closed

Provide simpler ScheduledTasks API #5301

tsteur opened this issue Jun 5, 2014 · 8 comments
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Jun 5, 2014

As part of our "convention over configuration" strategy we want to simplify the ScheduledTasks API. Currently you need to know how hooks/events work, you need to know about the details of 2 or 3 classes and how to handle them etc. In addition the plugin class gets bloated. Goal is to make it super easy but still provide huge flexibility and possibility for refactorings.

If possible provide a console generate command for scheduled tasks.

If - and only if - useful we could let people automatically execute the tasks via command line as well. The method name would be the command such as ./console tasks:privacymanager:deleteReportData. Via reflection we could automatically detect a possibly needed parameter and documentation. This is just an idea I don't think it is useful right now or is it? Just saying it is possible :)

@tsteur
Copy link
Member Author

tsteur commented Jun 5, 2014

In 3e03b0a: refs #5301 started to simplify scheduled tasks API while staying backwards compatible

@tsteur
Copy link
Member Author

tsteur commented Jun 5, 2014

In 756d1e0: refs #5301 added scheduled task generator, some code tweaks and more docs

@tsteur
Copy link
Member Author

tsteur commented Jun 5, 2014

In e65e378: refs #5301 this should fix the scheduledreports tests

@tsteur
Copy link
Member Author

tsteur commented Jun 5, 2014

In 5ac85b1: refs #5301 doc for plugin/tasks class

@tsteur
Copy link
Member Author

tsteur commented Jun 5, 2014

In b457fcc: refs #5301 make sure to pass the instance to ScheduledTasks and not the className. Otherwise it is called statically. The disadvantage of this way is all task will be stored under the name "Tasks.$methodName" which can lead to naming collisions very quickly if it is not already the case. I am wondering why we keep only the last part of the class name and not the full namespaced class name here: https://github.com/piwik/piwik/blob/master/core/ScheduledTask.php#L105 ? Keeping the namespace would fix the naming collision. If we change to full namespace it could be possible a scheduled task is not executed once as the name changes

@tsteur
Copy link
Member Author

tsteur commented Jun 5, 2014

Just noticed as the name of the task has already changed due to the refactoring we could easily change the behaviour of https://github.com/piwik/piwik/blob/master/core/ScheduledTask.php#L105 and use the full namespaced name as long as there are no objections?

@mattab
Copy link
Member

mattab commented Jun 6, 2014

+1 to use full namespace to avoid collisions!

@tsteur
Copy link
Member Author

tsteur commented Jun 6, 2014

In 89e8f8e: refs #5301 to prevent naming collisions use the full class namespace

@tsteur tsteur added this to the 2.4.0 - Piwik 2.4.0 milestone Jul 8, 2014
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Task Indicates an issue is neither a feature nor a bug and it's purely a "technical" change.
Projects
None yet
Development

No branches or pull requests

2 participants