|
thanks, going to test/review it later today
1/ I do not like the use of $CFG->forcehttpsforrss in rss/file.php - any session starting should be done by $SESSION global instance
we can not detect the https reliably in core now - major problems is the offloading of encryption to another server - my +1 to leave this out for now and fix the https handling in core first 2/ the rss should use contextid as module or plugin identifier now - we will be moving to this for all module/block files next week; the first implementation of new API should be finished in about 2 weeks from now 3/ rss_auth() does not respect the $CFG->guestloginbutton which also disables any guest login if disabled 4/ $CFG->forcelogin setting is not respected too I think 5/ calendar/lib.php - following if does not seem to be correct: 6/ why the changes in require_user_key_login()? 7/ please watch your whitespace, especially the trailing spaces and indentation 8/ do not initialise objects with NULL value 9/ use proper PHPdoc inline syntax to annotate all new functions/methods 10/ course_setup() must be used if you assign $COURSE global object directly 11/ I guess <br /> is not recommended if you want to separate the rss icons in footers, it might be hard to style with css Today I was thinking about possible RSS feed upgrade path in 2.0, the current feeds are going to break anyway - we could start using a new file /rssfile.php and let the old one rss/file.php just display feeds with nice error messages including info how to subscribe to new feeds. Is there any ETAG support planned? This could help with perf a bit, loading of user caps is not cheap. petr Thanks for the review Petr.
1) I didn't like it either, so I will be happy to remove this check 2) Can you tell me where I can find more info about this changes? 3) 5) 11) My mistake. I will fix it. 4) You are right. But I can't imagine how it works in context of our 'magic' urls. I think the use of this URL's already assumes 'logging in'. 6) I just wanted to pass user_key to this function directly. 7) I need to get rid of a habit of sleeping on a spacebar 10) Ok. By the way, I there is a theme_setup() call in course_setup(), can it cause any problems? You mean HTTP ETAG ? Yes, I was thinking about this. And IF-MODIFIED-SINCE too. 1) ok
2) http://tracker.moodle.org/browse/MDL-14589 4) forcelogin should imho mean that rss feed can not be accessed without key 10) I guess we will have to tweak require_login() somehow I have to work on getting 1.9.2 ready this week, next week I should be working on file related stuff again. Here goes update.
1)Tried to fix most of problems mentioned by Petr 2)Reformed RSS subsystem(Petr's idea about ETags).
Most of the times nothing changes in the feed - we do not have to send the actual feed content, we can just send HTTP 304 Not Modified header. And because no actual content is sent, this allows us to skip loading all capabilities, identifying users etc - improve performance. It may be convenient to prefetch some data in rss_newstuff(), that's why $cache is used. But there is a problem, when rss_new_stuff() result depends on what capabilities user has. In this situation we could: +Unread message RSS feed
Rsstype fields in DB for assigments and courses.
Latest version. I'll update specs page soon.
Btw, File API Docs page says something about old file.php used for compatibility. What exactly does it mean? Another update:
+file.php just displays error message How about merging it with HEAD? To get feedback and bug reports? Update to comply with HEAD.
Added the new files to the patch (no more need for zip file), and recreated the patch against latest HEAD (2008082602)
Petr, can you please put this on your list to merge into 2.0?
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Implement core functions ?
Secure existing RSS feeds in Moodle ?
Add option to force HTTPS for RSS feeds ?
RSS for Calendar(Upcoming events) ?
RSS for Recent Activity ?
MDL-12563fixed ?