RSS module for G2 in the works?

La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Fri, 2004-10-01 17:39

Hi,

I searched the forums concerning using RSS in G2. I've see posts saying it should be pretty easy to create a module for RSS. I've also read that RSS can be used in 1.4.4-pl2.

Is there such a module for G2? If not, is somebody working on it?

Thanks.

Carl

 
mindless
mindless's picture

Joined: 2004-01-04
Posts: 8601
Posted: Fri, 2004-10-01 17:50

No module yet, and I'm not aware of anyone working on it.. if anyone wants to take a shot at it just let us know.. we'll give you some pointers on constructing a G2 module.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Fri, 2004-10-01 18:58
mindless wrote:
No module yet, and I'm not aware of anyone working on it.. if anyone wants to take a shot at it just let us know.. we'll give you some pointers on constructing a G2 module.

Thanks for the quick response. I could consider trying to write the module. It shouldn't be that hard, right? :wink:

 
mindless
mindless's picture

Joined: 2004-01-04
Posts: 8601
Posted: Fri, 2004-10-01 20:45

Probably not too hard.. though I'm not up on RSS, so you'll have to help me understand what the module needs to do before I can help get you started on G2.. (or someone who knows RSS and G2 can chip in here.....) I haven't even used the feature in G1.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Sat, 2004-10-02 01:39
mindless wrote:
Probably not too hard.. though I'm not up on RSS, so you'll have to help me understand what the module needs to do before I can help get you started on G2.. (or someone who knows RSS and G2 can chip in here.....) I haven't even used the feature in G1.

Actually, I'm not even sure anymore it would have to be a "module".

It could be an independant PHP file that queries the database directly to get the description and URL of the last additions. This simple PHP script could have an optional argument to restrict the query to an album.

With my very limited understanding of G2 (I just installed it for the first time yesterday), I don't know what would be the benefits or the disadvantes of creating a module instead of a single PHP page.

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Sat, 2004-10-02 04:45
La_Maudite wrote:
It could be an independant PHP file that queries the database directly to get the description and URL of the last additions. This simple PHP script could have an optional argument to restrict the query to an album.

With my very limited understanding of G2 (I just installed it for the first time yesterday), I don't know what would be the benefits or the disadvantes of creating a module instead of a single PHP page.

You're right that you could create a simple PHP page to query the database. But you're really better off making it a module because then you can use the G2 framework which means that among other things:

  • You'll get cross-database cross-platform support mostly for free
  • Site admins will be able to add/remove support for your code
  • If you work with us, we can add it to the core distribution and it can be shipped as part of G2
  • You can a site admin configuration page for it and allow admins (or users) to create different RSS profiles for viewing the data (simple, advanced, etc).
  • We can create unit tests for your code to make sure that it continues to run reliably and doesn't regress
  • We can have our localization team translate it into many languages

If you're interested, come talk to us on the #gallery channel on irc.freenode.net, or keep posting here and we'll help.[/]

 
mindless
mindless's picture

Joined: 2004-01-04
Posts: 8601
Posted: Sat, 2004-10-02 05:01

Here's some places to look to get started..

  • modules/photoaccess/module.inc is a basic module definition.. create a new directory in modules and copy this file for your module.inc. Edit the obvious places; you can remove performFactoryRegistrations().
  • look at modules/core/classes/GalleryModule.class for info on module methods
  • look at modules/core/classes/GalleryCoreApi.class for the list of common api calls
  • for rss you'll probably want an immediate view to return the output.. look at modules/zipcart/Download.inc for an example of an immediate view.

after you define your view you can access it like this:
yourserver.com/gallery2/main.php?g2_view=modulename:ViewName[/]

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Sat, 2004-10-02 06:23
mindless wrote:
after you define your view you can access it like this:
yourserver.com/gallery2/main.php?g2_view=modulename:ViewName

OK, that work. Thanks! For now, I have an empty RSS file (no items) when I access my view. That's a good start :wink:

Now, what I have to figure out is how to get the la N items, their urls and descriptions. If you have suggestions where to look for examples, that would be appreciated. I'll be looking at the imageblock module tomorrow for inspiration.

 
mindless
mindless's picture

Joined: 2004-01-04
Posts: 8601
Posted: Sat, 2004-10-02 15:23

if you want a parameter to your view our standard one is g2_itemId, ie
g2_view=modulename:ViewName&g2_itemId=number
GalleryView has a convenience method to load that item:
list ($ret, $item) = $this->_getItem();
This loads the item id specified, or the root album of the gallery if itemId is not specified.
you can use GalleryUtilities::getRequestVariables to get any other query parameters you may need.
I think there's a GalleryCoreApi to get all children of an album, in case you need that. You can do $item->getCanContainChildren() on any item to check if you can retrieve it's children.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Sat, 2004-10-02 22:52
mindless wrote:
GalleryView has a convenience method to load that item:
list ($ret, $item) = $this->_getItem();
This loads the item id specified, or the root album of the gallery if itemId is not specified.

Ok, I'm getting closer to a first working prototype. I have plenty of ideas to make this more flexible, but I want to make a simple version first.

I tried to use the GalleryTemplate class to generate the XML using templates, but I wasn't able to make it work. So now I'm back to writing the XML using "print" commands. It's not bad because the format probably won't evolve anyway.

I have a question to be able to continue though. I see in the templates commands of this form.
{g->url arg1="view=core:ShowItem" arg2="itemId=`$block.id`"}

How can I do that in PHP? I'm sure there is a function in the Gallery API, but I just can't find it. In other words, I'm looking for a function that will give me an URL to the item given it's item ID. Can you point me to it? Thanks.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Sun, 2004-10-03 00:53

Answering to my own post... I know... :wink:

La_Maudite wrote:
I have a question to be able to continue though. I see in the templates commands of this form.
{g->url arg1="view=core:ShowItem" arg2="itemId=`$block.id`"}

How can I do that in PHP? I'm sure there is a function in the Gallery API, but I just can't find it. In other words, I'm looking for a function that will give me an URL to the item given it's item ID. Can you point me to it? Thanks.

I found it.

$urlGenerator = $gallery->getUrlGenerator();
$url = $urlGenerator->generateUrl(array('view' => 'core:ShowItem', 'itemId' => $itemId));

I should be able to get it to work now.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Sun, 2004-10-03 02:51

Ok, I got something that at least works for the top gallery and is recursive. I'm of course opened to discussion on improvements. It also works for any album if you know the itemId for that album. Of course, the best thing would be to have the module display a small RSS link for the current album. I also currently have an admin section that let's you choose the number of items that are going to show up in the RSS feed, but it's not easily accessible right now (have to type the URL :-\ ) and I can't fully test it because I only have a tiny gallery copntaining 3 photos :wink:

Actuallly, I've got a few things I'd like to see implemented to give more flexibility:

Options in the module admin:
1) Let the admin choose what type of items can be viewed. Currently, only GalleryPhotoItem can be viewed (hardcoded)
2) Choose between creation and modification timestamp

Options that could be passed as arguments:
1) Possibility of having a RSS feed for the comments
2) Choose if the RSS feed should be recursive or not. Currently recursive only.
3) Choose between title and summary for the RSS "title" tag. I really don't like the fact that G2 shows the filename, so I curerntly use the summary.

Possible optimisations:
1) I'm sure the code will be slow for huge galleries. The best way to approach this would probably be to have a direct call to MySQL/Postgres/Whatever that returns the right number of items in the correct order. But for now, I have done it the way it was suggested to me, which means that PHP is comparing timestamps.
2) We could probably use templates instead of "print" commands in PHP. I'm not sure we would gain anything from this though.

So, I could commit this stuff if you would like me to. Just let me know how to approach this. I could send you a tar.gz of my module if you want. Also, I wanted to say that I developped a lot by imitation (and mostly cut-paste-modify :wink:), so puting my name in the author section of the copyright feels a bit weird. Just let me know what I should do about this.

Looking forward to hear from you guys.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Sun, 2004-10-03 03:32

beckett asked me to give a link to the RSS module code:
http://www.carlrobitaille.org/downloads/rss-module-0.1.tar.gz

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Sun, 2004-10-03 07:12

I reviewed the code and sent La_Maudite a review. He'll work with us on improving the module so that we can get it committed.

 
mindless
mindless's picture

Joined: 2004-01-04
Posts: 8601
Posted: Sun, 2004-10-03 16:06

Take a look at modules/register/classes/GalleryPendingUserHelper::_sendEmail for an example of using GalleryTemplate directly.

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Thu, 2004-12-09 17:58

What's the status on this? </curious>

 
baschny
baschny's picture

Joined: 2003-01-04
Posts: 328
Posted: Fri, 2004-12-10 00:39

geddeth, there is a task for rss support, you can find a link to a forum topic there, and some other notes. There seemed to be some activity some months ago, but it dried out (see forum post), maybe you can revive it? ;)

Cheers,
Ernesto

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Fri, 2004-12-10 09:22

If it really has stagnated, I'll definetely do that. ;) What say you, La Maudite? Wanna hook up?

 
walkah
walkah's picture

Joined: 2004-03-11
Posts: 5
Posted: Fri, 2004-12-10 16:37

I have also looked at this, and am interesting and willing to help get rss support into G2.

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Sat, 2004-12-11 15:35

Great, so we have a team already! :) I have the resources available to set up a mailinglist and CVS module for development, but it may be a bit overkill for this. What do people think? We could continue where La Maudite left off (does anyone have anything newer than the link he provided from this thread?) and coordinate our efforts via the mailing list, and then hand the source over to the G2 devs when we have something solid.

Any other/better ideas?

 
baschny
baschny's picture

Joined: 2003-01-04
Posts: 328
Posted: Sat, 2004-12-11 21:54

geddeth, you could use gallery-devel mailing list or the forums to interact, no need to add yet another forum. Most fun happens on irc (#gallery on Freenode). If you come up with something following our coding guidelines, cover all your code with unit-tests, post it to the forums so we can review it and include that to G2's main CVS tree.

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Mon, 2004-12-13 15:56

Hi,

Thanks to geddeth for telling me that I should read this thread. I'll be glad to give the code away for you guys to continue the work. Unfortunately, I didn't have time to work on it for quite some time. But I have a better and working version of the RSS module. I had private discussions with Bharat, and that is why I haven't updated thread when I had the working prototype or reading the thread after that. Sorry about that guys and many thanks to geddeth for contacting me about it.

Here is a cut-paste of the message I sent Bharat on 2004-10-21:
=====================
I have reached a working prototype, at last. I wasn't expecting at first to
have different codes for the Photos and Comments and was trying to have a
single code. Spent some time before I realized that Comments are not Items
and that had major impacts. But now, I have a working RSS code for photos and
comments for a given album.

It's working, but I have with me a list of options to put in the Admin
section. I also have a some issues with the session, and Bharat knows about
it. Anyway, even if I'm not ready to release, I thought you guys might want
to see it working.

http://www.carlrobitaille.org/photos/main.php/view/?g2_page=1

Note that I just found out the the main page without the /view/?g2_page=1
doesn't show the links at the bottom of the left sidebar.
http://www.carlrobitaille.org/photos/main.php

It has probably something to do with not having an itemId in the arguments.
Anyway. If you go to any album (or sub-album) you'll see two new links at the
bottom left:

"RSS Photots"
"RSS Comments"

These are for the current album and all it's decendants. I decided to link the
new comments to the view image page instead of the comment page because there
wasn't any advantage to link to the former. I also have a "dummy" argument
"commentId" that is not used by G2 but distinguishes the links between
comments of the same photo. This is useful for HTML based RSS readers like
mine to know what you already read ;-)
http://www.carlrobitaille.org/rssfeeds/?id=CarlRobitaille.org%3A+UPPJ+photos

I'll let you know when I have all the options (and other stuff I have to put
in) in place. If you want to download the current code:
http://www.carlrobitaille.org/downloads/rss-module-0.2.tar.gz
=====================

I haven't worked on it since then, so you can take it as is. On the other hand, I've been using it for more than a month now and it works great for me. As I said in my email to Bharat, I had ideas of options and improvements I could put into the RSS module, but I haven't had time to do that. I could make a list of those improvements/options if you'd like to have them.

Hope this will be a good start for your work guys.

Cheers.

Carl

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Fri, 2005-01-07 16:02

Someone pointed out to me that the <lastBuildDate> and <pubDate> tags didn't have a good format. I fixed that (modified RSS.tpl and RSSItem.tpl) and made a new tar.gz.

rss-module-0.2.1.tar.gz

I didn't see the error before because I'm using a PHP script I wrote using MagpieRSS to read my RSS feeds, and I'm not using these tags.

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Sat, 2005-01-08 08:15

I finally got around to doing a full code review on your module. It looks really great; you've definitely gotten most of the principles of G2 down. I'd like to get your code into the main codebase but there are a few things that we need to clean up first. Most of my comments are relatively small cosmetic things revolving around style and naming. The main big thing is that we need unit tests for GalleryRSSHelper.class and AdminRSSController.

If you're low on time, we can help out with this. But it'll happen faster if you can push it over the finish line! Thanks!

GalleryRSSHelper.class:

  • Should be renamed to "GalleryRssHelper" to conform to our naming conventions (common acronyms are treated like words). In fact all uses of RSS should be Rss when used as a class or variable name.
  • 50: "retreave" -> "retrieve"
  • If you use "[GalleryEntity::entityType] = 'GalleryPhotoItem'" then you
    will only get photo items. You won't get subclasses of GalleryPhotoItem
    which means that any new extensions that people add (PanoramaPhotoItem)
    won't show up. Instead, you should join through the GalleryPhotoItem
    table because all subclasses will have a row there too. I think you can
    just add this to the where clause:
    [GalleryEntity::id] = [GalleryPhotoItem::id]
    and [GalleryPhotoItem] to the from clause and drop the entityType comparison.
    Looks like that's what you did in fetchLastGalleryComment

RSS.inc

  • 49: the /** */ style (two stars in the open comment) is a special trigger (this happens all over the module)
    for phpdoc; only use it when you're documenting a method, not inside a method.
  • 78: (and lots of other places). These lines should be localized. Get the module, like this:
    list ($ret, $module) = GalleryCoreApi::loadPlugin('module', 'rss');
    then translate all the strings:
    $module->translate('Missing itemId argument')
  • 117: When you translate the version, try to avoid concatenating your own strings; that doesn't localize well. Instead do $module->translate(array('text' => 'Gallery 2: RSS module version %s', 'arg1' => $version));
  • 145: proper style is "} else {"
  • 173,246: use GalleryCoreApi::requireOnce instead -- it'll help when we migrate to allowing modules to be downloaded and installed by the framework
  • 214,293: use GalleryCoreApi::relativeRequireOnce here (for the same reason) but use 'modules/core/classes/GalleryTemplate.class' as the path.
  • 218,297: If this was in a global template, then the template variable should be the same as the view, so it should be "RSS". You're not in a global template, but it's probably good to follow the same convention. Usually we call the variable the same name also ($RSS).
  • 220,299: use the full path (from gallery2) to the RSS.tpl here
  • 263: Is there any advantage to doing the reset/while/each instead of doing "foreach ($children as $keyComment => $comment)"?

RSS.tpl

  • our boilerplate header has changed, see a current template for a new example
  • 24: extra space before the }
  • Your data should all be in $RSS here.
  • title, description, language, generator, docs: none of these need to be localized. They're all raw content which can't be localized (right?).

RSSLinks.tpl

  • our boilerplate header has changed, see a current template for a new example
  • Your data should be in $RSSLinks, not $RSSSystemContent
  • "RSS Photos" and "RSS Comments" should be internationalized.
  • <br> should be <br/> (XHTML compliance)

RSSItem.tpl

  • our boilerplate header has changed, see a current template for a new example
  • Your data should be in $RSSItem, not $rssDataItem
  • 11: {if empty($RSSItem.commentId)} works too
  • description, author, album title don't need internationalization.
  • $rssDataItem.album_title should be $ressDataItem.albumTitle (for consistent naming)

AdminRSS.tpl

  • How about making the description:
    {capture name="url"}<a href="http://blogs.law.harvard.edu/tech/rss">{g->text text="Really Simple Syndication"}</a>{/capture}
    {g->text text="RSS is %s" arg1=$smarty.capture.url}
    

module.inc

  • in your constructor, you should have:
     $this->setRequiredCoreApi(array(1, 0)); 
     $this->setRequiredModuleApi(array(0, 8)); 
    

    (you can use earlier revisions, but those versions are current)

  • _loadRSSLinks() requires Phpdoc
  • 147: use GalleryCoreApi::requireOnce
  • 164: use RSSLinks instead of RSSSystemContent as the variable

[/]

[/]

[/]

[/]

[/]

[/]

[/]

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Sun, 2005-01-09 02:20

First things first: La Maudite has allowed me to continue work on his excellent RSS module. However, having no experience with the G2 API, I have spent quite a lot of time reviewing the G2 Development StarterKit, the Developer's Guide, the G2 API docs, other modules and of course La Maudite's RSS module itself in order to gain insight into this whole business. I found the hardest bit was to get acquainted with the Smarty API and G2's interaction with it, which is just another way of saying that the G2 API is well thought-through and relatively easy to comprehend.

That being said, I have far from understood all that is going on in this RSS module. So excuse me (Bharat especially) if I've been rather slow in getting this (otherwise simple) module out into the open again, after I promised to do so. I believe the steep learningcurve (and the holidays :lol:) were my major time-consumers in the past weeks.

So - on to business: the current status of the module can be found here, aptly version-bumped to 0.2.2. It is usable with current CVS, but retains the functionality that La Maudite described earlier in this thread. In module.inc, I have changed the way the (auto-)configuration of the module happens, since I believe 0.2.1 used an old method for this (pun intended ;-)). A diff between module.inc in 0.2.1 and 0.2.2 should reveal these changes.

Regarding your code review, Bharat, these are my comments (or questions, rather):

bharat wrote:
The main big thing is that we need unit tests for GalleryRSSHelper.class and AdminRSSController.

If you're low on time, we can help out with this. But it'll happen faster if you can push it over the finish line! Thanks!

I have not looked into this. I'd like to, but I feel that you are also interested in getting the module into CVS soon, in which case it would probably be faster if someone on the team could help out. In any case, I'll look into how this is done (and do it, I hope) within a week.

bharat wrote:
RSS.inc
  • 220,299: use the full path (from gallery2) to the RSS.tpl here

I've changed it to $template->fetch(dirname(__FILE__) . 'modules/rss/templates/RSS.tpl'); - I assume that is what is meant.

bharat wrote:
RSS.inc
  • 263: Is there any advantage to doing the reset/while/each instead of doing "foreach ($children as $keyComment => $comment)"?

I really think La Maudite is in a much better position to answer this than I. If no arguments against changing it appear, I'll do so.

bharat wrote:
RSS.tpl
  • title, description, language, generator, docs: none of these need to be localized. They're all raw content which can't be localized (right?).

Don't know what this means, sorry. I haven't quite grasped how localization works yet.

bharat wrote:
RSSLinks.tpl
  • "RSS Photos" and "RSS Comments" should be internationalized.

Don't know how this works, but have added the g->text function call to the template.

bharat wrote:
RSSItem.tpl
  • description, author, album title don't need internationalization.

I assume this means removing the g-> text function calls? This is not done yet.

The rest of the changes have been implemented, and the module seems to function properly, so at present I'm assuming I've done it right. However, I've only tested to see that the XML was created properly and updated along with the gallery, nothing else yet.

Lastly, thanks Bharat, for taking the time to do such a thorough review of the code, it gave me further insight into how the G2 API functions and is to be used.

Best regards,
G.[/]

[/]

[/]

[/]

[/]

 
La_Maudite

Joined: 2004-10-01
Posts: 13
Posted: Mon, 2005-01-10 13:51
geddeth wrote:
bharat wrote:
RSS.inc
  • 263: Is there any advantage to doing the reset/while/each instead of doing "foreach ($children as $keyComment => $comment)"?

I really think La Maudite is in a much better position to answer this than I. If no arguments against changing it appear, I'll do so.

I've looked at it and the reason I wasn't using a foreach is that I didn't know foreach and each mixed well. As you can see in the line following the while statement I'm doing calling each on $children. See the comment on GalleryRSSHelper::fetchLastGalleryComment to see how the data is returned:

     * @return array (object GalleryStatus a status code,
     *                array (GalleryComment, GalleryItem, GalleryComment, GalleryItem, ...)

I'm sure the data could be returned differently to be able to do a foreach. Also, maybe the each statement isn't incompatible with the foreach. Not coding in PHP frequently, I'm not sure about the latter.[/]

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Tue, 2005-01-11 01:58

rss-module-0.2.3.tar.gz

CHANGELOG:
----------
 * 2005-01-11: 0.2.3 released: now validates, some bugs removed
 * 2005-01-11: Minor code layout/comment cleanup
 * 2005-01-11: Fixed PHP syntax error in RSS title display
 * 2005-01-11: Added creationTimeGMTOffset to RSSItem.tpl in order
               to show pubDate in GMT and comply with RSS 2.0 and RFC 822
 * 2005-01-11: Used getHttpDate() instead of time() in RSS.tpl in order
               to show lastBuildDate in GMT and comply with RSS 2.0
               and RFC 822[1]
 * 2005-01-11: Set default mail address for comments without one in order
               to comply with RSS 2.0
 * 2005-01-11: Added e-mail to <author> in order to comply with RSS 2.0
 * 2005-01-10: Fixed a syntax error in the resulting XML (<item>s not
                                                          inside <channel>)
 * 2005-01-10: Modified the RSS links (added small RSS logo)
 * 2005-01-09: 0.2.2 released; now seems to work with CVS (minor bugs aside)

[1] Strictly speaking, it's RFC 2616 compliant, but the feed validates.
    Are there problems regarding RSS dates and RFC 822 vs. 2616?


TODO/ISSUES:
------------
 * Add option for NOT subscribing to ChildEntities
 * Add RSS links to front page as well (also when main.php is disabled)
 * Find out how to replace XML encoding (currently ISO-8859-1)
   and <language> with current G2 equivalents
 * Ditch Dublin Core metadata? No dc-elements are used (yet).
 * If several image resolutions exist of the same picture,
   which do we link to?
 * Can we link more directly to specific comments, instead of just going
   to the album they're in?
 * Should the current Gallery name be added to the feed title?

Comments are more than welcome. :wink:

BTW: Anonymous comments don't have an e-mail address, but they have to for the comments RSS feed, if we want it to validate. Now it defaults to

. Aside from the fact that default e-mail addresses Are Really Bad, what other options do we have?

Cheers,
G.

 
fryfrog

Joined: 2002-10-30
Posts: 3236
Posted: Tue, 2005-01-11 02:17

Do you happen to know the email address of any very (un)popular spammers? Maybe we could contribute to the email they get from site harvesters :)

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Thu, 2005-01-13 07:28

This is looking really good! It's getting pretty close. We still do need unit tests, and I don't think that I'm going to have a lot of time to write them myself (I'm now spending most of my evenings doing code reviews .. that's a good thing but it's not leaving me much time to code!). They're not all that hard; if you can take a shot at them I'll be happy to give lots of guidance.

General:

  • All uses of RSS should be Rss since we're treating commonly used acronyms as words. I alluded to this in my last review but didn't really spell it out -- sorry! So RSS.inc should be Rss.inc and it sohuld contain RssView and the template variable its in should be $Rss and its template file should be Rss.tpl. You get the idea. Naming consistency is very important in loosely typed languages like PHP. I should have totally picked up on this in my first review but, uh .. it was late. I was tired. There was a tornado! Or something :-) For this review, I'll use the names that you're currently using so that it's not confusing.

GalleryRssHelper.class:

  • The GalleryRssHelper class was renamed, but the file should also be renamed to match for consistency.
  • 70: should be indented to 12 columns
  • 152: "retreave" -> "retrieve"
  • 97,99: you're in a sprintf here, so why not replace $groupIdMarkers and $andPermission with %s and make them arguments to the sprintf instead? Same for $direction at the bottom. As long as we're using sprintf, let's use it all the way. The same applies to fetchLastGalleryComment()
  • fetchLastGalleryComment() isn't the most appropriate name. How about fetchRecentGalleryComments() instead? And likewise for fetchLastGalleryPhotoItem()?
  • in fetchLastGalleryComment, the way that you return interleaved GalleryComment and GalleryItem instances here leads to the unusual use of reset()/each() calls in RSS.inc. It's also inefficient since if you have 10 comments for 1 item you're going to add the item to the $entityIds array 10 times (once for each comment) then load it 10x extra times in GalleryCoreApi::loadEntitiesById(). A better approach would be:
            /* Get all of our comment ids and item ids */ 
            $entityIds = array(); 
            while ($result = $searchResults->nextResult()) { 
                $commentIds[] = $result[0];  /* keep these in order */
                $itemIds[$result[1]] = 1;    /* make a hash table of items */
            }
    
            /* Convert them to entities */ 
            if (sizeof($commentIds) > 0) { 
                list ($ret, $comments) = GalleryCoreApi::loadEntitiesById($commentIds); 
                if ($ret->isError()) { 
                    return array($ret->wrap(__FILE__, __LINE__), null); 
                }
                list ($ret, $items) = GalleryCoreApi::loadEntitiesById(array_keys($itemIds)); 
                if ($ret->isError()) { 
                    return array($ret->wrap(__FILE__, __LINE__), null); 
                }
                foreach ($items as $item) {
                    $itemIds[$item->getId()] = $item;
                }
            } else { 
                $comments = array(); 
                $items = array(); 
            } 
     
            return array(GalleryStatus::success(), $comments, $items); 
    

    Now we have all the comments (in the order that we wanted them) and we have all the items that the comments will refer to (unordered, but we don't care about the order). Then in RSS.inc:

            if ($entity == null) { 
                $comments = array(); 
            } else { 
                list ($ret, $comments, $items) = 
                    GalleryRSSHelper::fetchLastGalleryComment($itemId, $maxItems); 
                if ($ret->isError()) { 
                    return $ret->wrap(__FILE__, __LINE__); 
                } 
            } 
     
            $RSS['items'] = array(); 
            foreach ($comments as $comment) {
                $RSSItem = array(); 
                $commentMemberData = $comment->getMemberData(); 
                $itemMemberData    = $items[$comment->getParentId()]->getMemberData();
      // (rest is the same)
    

    This way we iterate over the comments, and then when we want to get the parent item's data, we grab the parent from the parent's hash map and use it.

RSS.inc

  • 53,63,129: multiline comments should be in the form:
    /*
     * line 1
     * line 2
     */
    
  • 120: use ' instead of "
  • 134, 141: these strings need to be localized. In general, concatenating strings like this is going to make it very difficult on the localizers because they prefer to work with the whole block of text, not little bits of it. But for now you can do: $RSS['title'] = $module->translate(array('text' => '%s (Photos)', arg1 => $RSS['title'])), etc.
  • 256-258: following up on my prior review (and the responses) you should use:
    foreach ($children as $keyComment => $comment) {
    

    here instead.

  • 286: use ' instead of "
  • 208-223, 293-308: the XML rendering is duplicated in your two render methods. Have your methods return the $RSS object, then create and render the template in renderImmediate() and you'll save some code duplication.

RSS.tpl

  • <description>{g->text text=$RSS.description}</description> should be <description>$RSS.description</description> since the description is pure content; it can't be localized. Anything in {g->text} blocks must be a constant that can be separated out and passed to localizers for translation. Same for language, docs and generator, they should be:
        <language>en-us</language> 
        <docs>http://blogs.law.harvard.edu/tech/rss</docs> 
        <generator>$RSS.generator</generator> 
    

    language and docs are constant. They can't be translated. generator is pure content from the user like the description.

RSSLinks.tpl

  • Indentation in HTML files is two spaces, so it should be like this:
    <div>
      <ul>
        <li>
          ..
        </li>
      </ul>
    </div>
    
  • The <img> urls should be:
    {g->url href="modules/rss/images/rss.gif"}
    

    which means that you don't need $RSS['moduleUrl'] in module.inc

  • Extra blank lines can be deleted (so that we send less bytes to the browser).

RSSItem.tpl

  • description, email, author, albumTitle should not be in {g->text} blocks

module.inc:

  • autoConfigure() should not set the version. We used to do that a while back but I moved all of that up into the GalleryModule class. Look at the zipcart or netpbm modules for an up to date example of how autoConfigure should look. Basically you can delete all version related lines and keep everything else
  • 99: use /* instead of /**
  • 169-170: inconsistent whitespace on these two lines
  • 172: moduleUrl can be deleted (as I mentioned above)

[/]

[/]

[/]

[/]

[/]

[/]

[/]

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Thu, 2005-01-13 10:53

Thanks, Bharat, for yet another great review to work with. I'll get cracking on it instantly, including the unit tests.

bharat wrote:
All uses of RSS should be Rss since we're treating commonly used acronyms as words. I alluded to this in my last review but didn't really spell it out -- sorry! So RSS.inc should be Rss.inc and it sohuld contain RssView and the template variable its in should be $Rss and its template file should be Rss.tpl. You get the idea. Naming consistency is very important in loosely typed languages like PHP. I should have totally picked up on this in my first review but, uh .. it was late. I was tired. There was a tornado! Or something :-) For this review, I'll use the names that you're currently using so that it's not confusing.

I totally understand the PHP-related argumentation and will change the modulenames, functions and variables, but do you also mean it to include PhpDocumentor tags (i.e. @package and @subpackage) and RssModule->setName('Rss')? After all, the common practice is to write abbreviations like that in capitals, and the EXIF module seems to stick to this notion as well.

G.

Edit: Scratch that last question, I get it now :)

 
crschmidt
crschmidt's picture

Joined: 2002-12-12
Posts: 4
Posted: Fri, 2005-01-14 01:51

geddeth: regarding the encoding: Gallery ensures that the encoding is always UTF-8 (at least, the Global template seems to assume so.) Therefore, iso-8859-1 can simply be replaced with utf-8, or, if you prefer, left out of the prolog altogether (making it <?xml version="1.0" ?>.) The default encoding for XML is utf-8, so leaving it off is the same as changing it.

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Mon, 2005-01-17 13:34

crschmidt, I followed your recommendation and eliminated the string completely. Seems to work fine, also with e.g. Danish vowels (æøå).

G.

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Mon, 2005-01-17 13:38
bharat wrote:
[*] 256-258: following up on my prior review (and the responses) you should use:

foreach ($children as $keyComment => $comment) {

here instead.

I don't quite understand. In the beginning of your post, you suggest it be altered to foreach ($comments as $comment) { which seems to work fine?

The other issues you mentioned are all implemented now.

Also, is there anywhere I can look for directions regarding unit tests? I've searched the forums, read the Developer's guide and am now checking out source (core->AdminCreateGroupControllerTest among others), but I really feel a bit in the dark here, possibly because I've never worked with XP before.

I'll keep cut'n'pasting for now, maybe something will work.

Cheers,
G.

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Mon, 2005-01-17 15:27
geddeth wrote:
I'll keep cut'n'pasting for now, maybe something will work.

And indeed it did .. it seems I have a working unit test for the admin module (not sure if I need more). I was figuring out why it kept failing, and eventually removed the line: $results['return'] = 1; from handleRequest(), since as far as I could gather, it had no function. Please correct me if I'm wrong. :D

Anyway, the unit test is extremely simple. Guess that's because the module itself is actually not very complex.

The module has been version-bumped and is availabe here:

rss-module-0.2.4.tar.gz

Here's a snippet from the README inside the archive:

CHANGELOG:
----------
 * 2005-01-17: 0.2.4 released
 * 2005-01-17: Added unit tests for the admin module
 * 2005-01-17: Left out encoding info in XML, so it always defaults to UTF-8 (thanks, crschmidt)
 * 2005-01-17: Fixed bug in generation of <generator>-tag
 * 2005-01-17: More minor code cleanups; _loadRssLinks() merged with loadSystemContent()

TODO/ISSUES:
------------
 * Add option for NOT subscribing to ChildEntities
 * Add RSS links to front page as well (also when main.php is disabled)
 * Find out how to replace content of <language> with relevant G2 setting
 * Ditch Dublin Core metadata? No dc-elements are used (yet).
 * If several image resolutions exist of the same picture, which do we link to?
 * Can we link more directly to specific comments, instead of just going to the album they're in?
   For now, we can add #gsComments to the link, but if the commentId was added to the HTML, we
   could jump directly to it on pageload (ie. #g2_commentId_368 or similar).
 * Should the current Gallery name be added to the feed title?
 * Variable names can be more consistant and informative
Suggestions from La_Maudite:
 * Let the admin choose what type of items can be viewed. Currently, only GalleryPhotoItem can be
   viewed (hardcoded)
 * Admin: Choose between using creation and modification timestamp
 * Choose if the RSS feed should descend into child albums or not
 * Choose between title and summary for the RSS "title" tag. I really don't like the fact that G2
   shows the filename, so I curerntly use the summary.

Since I took over development from La Maudite, I've focused only on streamlining the code in order to make it fit for inclusion in G2. Thus, I've not added any new functionality or altered the existing. Instead, I've collected my own thoughts and La Maudite's (see earlier in this thread) for a future revision, as you can see from the README above. I'd be happy to hear (more) comments and ideas for the module, so that there is something to work with when the time is ripe to expand the functionality.

Cheers,
G.

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-01-19 05:08

This is looking great! Looks like you rolled in all of my suggestions and are pretty close to the end. My review comments are of a much more subtle nature now. I think that we're getting pretty close to the point where we can commit this. I think that will make everybody very happy :-)

General

  • Detect if the comment module is active.
  • Add yourself as an @author everywhere.
  • I get warnings if I try the comment feed when I have no comments. Make sure that your system is configured to be strict according to our dev env spec so that you see these too.
  • Comments RSS won't work if the comments module isn't active. The easy fix for this is to disable the comment RSS button if the comment module isn't around. The quick and dirty way to do this is to try to detect the presenve of GalleryCommentItem, which you can do by asking the factory for it:
    list ($ret, $comment) =
       GalleryCoreApi::newFactoryInstance('GalleryEntity', 'GalleryComment');
    if ($ret->isError()) {
       // return $ret->wrap..
    }
    if (!isset($comment)) {
       // no comment module
    }
    

    The long term right solution for this is to create an interface in the GalleryComment module and move the fetchRecentGalleryComments API method into it, then create an implementation of that interface and publish it. This is better because it means that the comment module can change around as much as it wants, and you don't have to try to keep up with its database structure. Let's go with the quick hack for now and then after the RSS module gets committed we can start the process of moving the API method over. It won't take all that long, but it's an easy thing to do afterwards.

AdminRssControllerTest.class

  • Instead of doing
    $this->assertEquals(array(GalleryStatus::success(), $numberOfItems), 
     GalleryCoreApi::getPluginParameter('module', 'rss', 'numberOfItems')); 

    you should do:

    list ($ret, $savedValue) = GalleryCoreApi::getPluginParameter('module', 'rss', 'numberOfItems');
    if ($ret->isError()) {
      return $this->failWithStatus($ret->wrap(__FILE__, __LINE__));
    }
    $this->assertEquals($numberOfItems, $savedValue);

    It's a better pattern and does the error handling correctly. I used to do it the way you have above, but I've been updating that code wherever I find it.

  • Add a test where form[numberOfItems] = -5. What should happen?
  • Add a test where form[numberOfItems] = some random string (eg, "foo"). What should happen?

AdminRss.inc

  • 49: $error is going to be empty (you just defined it above as empty) so there's no point in checking it here.
  • 63: You can delete the "$results['return'] = 1" line altogether.
  • When you write the tests I suggest above, I think you'll realize that you have no error checking in your controller and are putting whatever the browser gives you right into the database. This is a bad idea, and the new unit tests should help make sure that it doesn't happen.

GalleryRssHelper.class

  • Both these helper methods should have unit tests. You can do this pretty easily by creating a little mini tree of albums, data items and comments, then calling the helper methods with the root album id and asserting that you get the right data back. Try removing view permissions from some of the items and make sure that they don't show up in the output. These tests become what we call an "executable design document" for your helper class -- they define the inputs and outputs for your method. Typically we try to write one test that covers the 80% case (all the standard stuff) then one test each for individual edge cases (invalid $itemId, the cases when there are no comments, no photos, etc). Then later on when we refactor this code (which we probably will -- it happens sooner or later) we'll know that we didn't break anything. Look at the setup method in modules/thumbnail/test/phpunit/CustomThumbnailOptionTest.class to see how you can create an album with a GalleryPhotoItem and a GalleryUnknownItem in it. Create a mix of those, maybe some sub-albums and then look in modules/comment/test/phpunit/CommentSearchTest.class to see how to create some comments. If the comment module isn't active, of course this is going to fail.
  • I really like how you used the %1$s notation in your sprintf() calls -- I hadn't thought to do that -- very clever!
  • in the SQL in fetchRecentGalleryPhotoItems you don't need to join through the GalleryEntity table. If it's in the GalleryPhotoItem table then you're all set. So basically you can convert all references to GalleryEntity to GalleryPhotoItem and you're all set. The SQL would look like this:
                SELECT DISTINCT 
                  [GalleryPhotoItem::id], [GalleryEntity::creationTimestamp] 
                FROM 
                  [GalleryPhotoItem], [GalleryPermissionMap], 
                    [GalleryItemAttributesMap=1], [GalleryItemAttributesMap=2] 
                WHERE 
                  [GalleryPermissionMap::itemId] = [GalleryPhotoItem::id] 
                AND ( 
                        [GalleryPermissionMap::userId] = ? 
                        OR 
                        [GalleryPermissionMap::groupId] IN ( %1$s ) 
                    ) 
                AND 
                  %2$s > ? 
                AND 
                  [GalleryItemAttributesMap=2::itemId] = ? 
                AND 
                  [GalleryItemAttributesMap=1::parentSequence] LIKE %3$s 
                AND 
                  [GalleryItemAttributesMap=1::itemId] = [GalleryPhotoItem::id] 
                ORDER BY 
                  [GalleryEntity::creationTimestamp] %4$s, [GalleryPhotoItem::id] %4$s

    I tested this out locally and it seems to do the same thing. This is the kind of refactor that's really easy to do after you have unit tests because if the unit test is well written, then you can tinker with the SQL as much as you want as long as the test passes.

  • 138: should be indented to 16 columns
  • in fetchRecentGalleryComments() I think you can also get away without joining through the entity table. Where you use [GalleryEntity::id] you can use [GalleryChildEntity::parentId] instead. I didn't try this out locally, but I can't think of a reason why it won't work.
  • All the error returns in fetchRecentGalleryComments() are still returning only 2 args, instead of 3 which means that if there is an error the calling code in Rss.inc will bomb.

Rss.inc

  • 54,55,56 (and other places): in multiline comment blocks the *'s should line up:
    /*
     * like this
     */
    
  • 105,106,205,206,214,215,263,264,277,278: the entity data should already be utf8 entities so unless I'm missing something, you don't need to call htmlspecialchars() on it here. Try putting something like "<my title>" into the title of an album and then viewing the RSS feed to see what I mean
  • 129: should be the multiline comment block style shown above
  • 177: the phpdoc should reflect that you're returning two values now
  • The error returns in both _renderXxx() methods should be returning an array now
  • the for() loops in _renderImmediatePhotos() and _renderImmediateComments() should both have a $gallery->guaranteeTimeLimit(5) in them to make sure that they don't run out of time while rendering.

module.inc:

  • 123: avoid catching and swallowing errors unless you know *exactly* why the error was thrown. Even then you have to do this with care. If there's an error in that call, you should let it bubble up to the framework and it will Do the Right Thing. Part of why you're getting this error is probably because $itemId isn't set to anything in some cases. See my next comment.
  • in loadSystemContent(), the 'itemId' param won't always be set because it's not in the url for the top of your Gallery. If it isn't set, you should load the root album id instead. See GalleryView::_getItem() for an example of how to do that. This will give you the buttons on the main Gallery page, too.
  • in loadSystemContent() you fetch the moduleParams but don't use them. This is a high-traffic method, so you should aim to streamline it where possible.
  • If the $entity isn't an GalleryAlbumItem, then $tpl will be undefined and will give a warning when you try to return it.

RssItem.tpl

  • Clever use of the GMT offset in your timestamp format!

[/]

[/]

[/]

[/]

[/]

[/]

[/]

 
geddeth
geddeth's picture

Joined: 2004-05-16
Posts: 52
Posted: Wed, 2005-02-09 22:18

Just so noone thinks I've forgotten: I'm still working on this. Real Life (tm) has prevented me from doing much progress lately, but it's coming along ;)

G.

 
lixp

Joined: 2005-02-03
Posts: 5
Posted: Fri, 2005-02-11 08:16

i have downloaded this module,
but how can use it ?
there is no rss.php file.
who can help me ?

 
valiant

Joined: 2003-01-04
Posts: 32509
Posted: Fri, 2005-02-11 08:27

lixp, I didn't try this module but usually, you just extract the module in the /modules folder of your G2. then browse to site admin -> modules and activate (& configure) the module.

 
lixp

Joined: 2005-02-03
Posts: 5
Posted: Fri, 2005-02-11 08:58

i want to use gallery2 in plog. In gallery1.44, there was a rss.php file in gallery folder. and i can use rss.php to get some album items in my plog.
but in gallery2,things changed.

 
lixp

Joined: 2005-02-03
Posts: 5
Posted: Fri, 2005-02-11 09:12

Rss 0.2.4 Adds RSS feeds to your albums and comments
Incompatible module!
Core API Required: 1.0 (available: 0.8)

where can i get core api 1.0?

 
valiant

Joined: 2003-01-04
Posts: 32509
Posted: Fri, 2005-02-11 12:09

on this website, there is a menu on the upper left corner. click on "Download Now!" and in the G2 paragraph, click on jmullans nightly snapshots link.
Download the most recent nightly snapshot of gallery2 and there you are.
If your current version is newer than G2 alpha 2 (or was it 4?),you can upgrade your current installation. If it is older, you'll have to install everything again. How to upgrade is described in the forums.

 
lixp

Joined: 2005-02-03
Posts: 5
Posted: Fri, 2005-02-11 12:31

thxs.
and now ,
the following :
Rss 0.2.4 Adds RSS feeds to your albums and comments
Incompatible module!
Core API Required: 1.0 (available: 3.2)
Module API Required: 0.8 (available: 0.9)

 
Raven69
Raven69's picture

Joined: 2004-01-16
Posts: 24
Posted: Fri, 2005-02-11 13:42

Wanted to download 'rss-module-0.2.4.tar.gz', but cannot connect to the server.
anyone out there who can e-mail me or provide it for download?

thanx in advance

RaVen

 
lixp

Joined: 2005-02-03
Posts: 5
Posted: Sat, 2005-02-12 01:57

i use the gallery2 latest version.
but cannot use rss module.

Rss 0.2.4 Adds RSS feeds to your albums and comments
Incompatible module!
Core API Required: 1.0 (available: 3.2)
Module API Required: 0.8 (available: 0.9)

who can help me ?

 
fryfrog

Joined: 2002-10-30
Posts: 3236
Posted: Sun, 2005-02-13 00:35

lixp, you should probably just not worry about it until its included in the g2 releases. It looks like the module wants an older version of module api, so i'd suggest just not worrying about it.

 
h0bbel
h0bbel's picture

Joined: 2002-07-28
Posts: 13451
Posted: Tue, 2005-02-15 19:34

I haven't been following this thread very closely, and I might be repeating something someone has already asked for, but here it goes anyway.

I would love it if the url for the rss feed makes it possible to change what the output of it is. By that i mean that an url like http://example.com/g2/rss.php?album=test&show=all will feed me with an rss including all images (if anon users have permissions to do so in the given album) and that http://example.com/g2/rss.php?album=test&show=highlight only gives the current highlight for the album. In the same context, a &show=latest will show the last X (configurable) uploaded images to the album.

That way, one could incorporate album's into another page, completely separate from the Gallery install. My idea is to do this in Wordpress, where I can post something about my new pictures, and include the thumbnails, with links, in the post itself, without having to hack Wordpress or Gallery code at all.

Also, this might be a nice way for G2 to be able to support a "Gallery of Gallery installs" as well, grab the RSS feed from a bunch of installs and display them in an album on your own install? G2 would have to incorporate it's own feed-reader for that to work though. Magpie RSS looks like a likely candidate for a module?

 
volksport
volksport's picture

Joined: 2002-10-06
Posts: 239
Posted: Tue, 2005-02-22 00:11

much love homey. ttys

 
iross

Joined: 2005-03-21
Posts: 10
Posted: Mon, 2005-03-21 19:49

Is there any chance this module could work towards the same format as the RSS support in Gallery v1. Many people use RSS from gallery 1 to integrate with blogs / other parts of their sites etc.. Seems sensible.

I'd work on it myself but I havent got that great an understanding of Gallery 2 yet, also I dont want to duplicate efforts...

Otherwise is there a quick hack that will add the thumbnail url to the existing feed in Gallery 2?

- Ross

 
volksport
volksport's picture

Joined: 2002-10-06
Posts: 239
Posted: Mon, 2005-03-21 20:20

I have taken this over, the two other who have worked on it have donated the code to us.

I will most likely be redoing a majority of the code, and my goal is to be able to support any type of rss feed (0.8, 0.9, 2.0 ... etc).

I would also like to let the feed be customizable. I begin work this week, so if you have any ideas or comments please get them to me sooner rather than later.

I really don't have much experience with RSS, but I think I can make this work for everyone.

Thanks

 
d3vlabs

Joined: 2005-05-10
Posts: 100
Posted: Tue, 2005-05-10 21:13

I'm fighting the same battle as those two above me struggling with the wrong API version. Isn't there any way to fix those for those using the latest version of G2 (beta 4 is it?). Can I update the API modules by hand or can I edit the .ini file or something like this to fit the requirements? This module would be really usefull to me right now and I wish that I didnt have to wait for the final version of G2 to use it. Thanks for your time and support.

" Rss 0.2.4 Adds RSS feeds to your albums and comments
Incompatible module!
Core API Required: 1.0 (available: 5.1)
Module API Required: 0.8 (available: 0.10) "

 
bharat
bharat's picture

Joined: 2002-05-21
Posts: 7994
Posted: Wed, 2005-05-11 07:46

You can update the RSS module to conform to the latest API. We document all of our API changes so you have to go through the module and figure out what needs to be changed to conform to the new APIs. You don't need to wait for G2 final for this, you can fix it today.