mod_rewrite

In Site Admin -> General there is a URL Style option where you can choose to use short URLs. that shoud help you some-what. If you want more than that you'll probably have to modify.

well, the "URL Style" option only formats the url like the following:

http://www.websites.com/gallery2/main.php/view/glenn/

I guess I take it I'll have to muck around w/mod_rewrite then?

instead of http://yoursite.com/gallery2/main.php/view/glenn/ why not use something shorter like http://yoursite.com/gallery2/x/glenn/ or http://yoursite.com/gallery2/view/glenn/ or something like that?

my $0.02,
keith

replicant, see my earlier post. I realize that wasn't totally clear -- but the point is that we (the dev team) want to write a module to maintain the .htaccess file for you and update the url generator appropriately. It's possible. Won't be trivial though.

if it's just for one URL it would be easy enough ;-) Or are ya'll wanting to make this an option for all albums in the gallery?

Allright, I think I've got a way to use mod_rewrite and be somewhat compatible with Gallery1 url's.

This is what a rule could look like, ie:
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^([^\.\?/]+)/$ main.php/view/$1 [QSA]

As for the URLGenerator class I override two functions. First of all, getCurrentUrl(). This function originally uses $_SERVER['REQUEST_URI'] but what we realy need is the actuall adress since it's hidden behind the RewriteRule, thus falling back on the $_SERVER['SCRIPT_NAME']. Secondly, the new URLs needs to be shortened. I simply strip out /main.php/view/ from parent::generateUrl(...).

What it all comes down to in the end is that I dont know how to register my new UrlGenerator soon enough. As described in my erlier post many URLs are already created before my class is even registrated.

bharat, a little hint would be nice. Or maybe I'm way off base with thiss idea.

It looks like your rewrite rule at least partially works, so that's pretty cool. For experimentation purposes, I'd start by just modifying the core GalleryUrlGenerator until you get it working. Once we have something that works reliably, then we can worry about trying to extract it out into a module and register it properly. I'll look into that in the meantime...

nicokaiser, right, thats why you need to make the UrlGenerator discharge $_SERVER['REQUEST_URI']. Edit /moules/core/class/GalleryUrlGenerator.class at line 131 where getCurrentUrl() starts. This is what the function should look like:

    function getCurrentUrl() {
	if (!empty($_SERVER['SCRIPT_NAME'])) {
	    $path = $_SERVER['SCRIPT_NAME'];
	    if (!empty($_SERVER['PATH_INFO']) && $_SERVER['PATH_INFO'] != $path) {
		$path .= $_SERVER['PATH_INFO'];
	    }
	}

	return $this->makeUrl($path);
    }

Now gallery realy knows where it's at.

Then edit line 621 (of the original file) where you should see a return statement like this:

return $url;

Make it look like this instead
return str_replace('/'. $this->_baseFile .'/view/', '/', $url);

This is what my .htaccess looks like:

RewriteEngine On

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule ^(.+)$   main.php/view/$1   [QSA]

Make sure you have Short URLs turned on.

Edit:
bharat, I've got a moduel to handle the .htacces for me, a class that extends the UrlGenerator, all I need is to register the class now.

Einstein, it should work with diffrent baseFiles. I haven't had the chance to test it though.

bharat,
The module only works if I edit the original GalleryUrlGenerator.class not to use $_SERVER['REQUEST_URI']. Some URLs (like the style sheet links and current view) are compiled before my module gets the chance to register its own UrlGenerator. With this fix the links get the right base, and correct view.

Fortunate enough, the album and file URLs are generated later on, with the new UrlGenerator.

When the module is activated URLs like http://e.com/gallery2/main.php/v/album/sub/ becomes http://e.com/gallery2/v/album/sub/ and http://e.com/gallery2/main.php/d/2691-1/picture.jpg becomes http://e.com/gallery2/d/2691-1/picture.jpg . This way any registered prefix works so that other modules can use mod_rewrite too.

Fix for the module to work:

in modules/core/classes/GalleryUrlGenerator.class at line 131:
replace getCurrentUrl() with this:

    function getCurrentUrl() {
	if (!empty($_SERVER['SCRIPT_NAME'])) {
	    $path = $_SERVER['SCRIPT_NAME'];
	    if (!empty($_SERVER['PATH_INFO']) && $_SERVER['PATH_INFO'] != $path) {
		$path .= $_SERVER['PATH_INFO'];
	    }
	}

	return $this->makeUrl($path);
    }

nicokaiser.
My guess is that the module doesnt get the chance to set the new UrlGenerator before the redirect url is created.

If only here was a proper way of overriding the default class... or its me who have missed some function somewhere. Maybe bharat could point me in the right direction? Or anyone else of the G2 developers for that matter.

rewrite 0.0.2 does not work for me (Apache 1.3.x, PHP 4.1.x), it displays a blank page when I try to install.

BTW, you are using "file_get_contents" which is not available in PHP < 4.3. This should be included to keep compatibility:

<?php
if(!function_exists('file_get_contents')) {
   function file_get_contents($file) {
       return implode('', file($file));
   }
}
?>

This looks like an excellent start! I have a fair number of comments below.

General

• We should probably create a new namespace for the urls we create because otherwise we'll wind up with possibly overlapping namespaces. For example, we have an actual file on disk called "themes/matrix/styles/theme.css". But we can also create a themes album, containing a matrix album, containing a styles album containing a file called "theme.css". The urls for the two would be exactly the same in your scheme and there'd be no way to differentiate them. I think that we should instead create a virtual namespace, like: http://your.server/gallery2/-/album1/album2/foo.jpg ..e by adding the /-/ we make sure that there's no conflict.
• Missing unit tests. Their lack is really noticeable for RewriteUrlGenerator.class because it's hard to understand the intention of some of the code (like why you're removing the query string, etc) without seeing what edge cases are covered. All the controllers and helper methods should be tested.
• Messing with the .htaccess file can lead to 500 errors on servers that don't want you to even create such a file. We need to provide a strategy to let users find out whether their server supports .htaccess, allows you to put stuff in it, and supports mod_rewrite. If we create a gallery2/.htaccess file on a server that doesn't support it then the whole app is dead in the water. The problem is that the only way to know for sure that it's going to work is to actually test it. In G1, we put a .htaccess file in the setup directory and actually tried it to see if it worked as part of the config wizard. However, this leads to the problem that on some subset of installations you get a 500 error just trying to use the config wizard. I think that a better approach is to create a series of directories under the test directory, each with a different .htaccess file in them. Then in the configuration page we provide links to those directories (using {g->url href="modules/rewrite/test/1"} etc) and we ask the user to click them. Or maybe even better we can have iframes inside the config view that loads them all for the user. If the page returns a 500 error then we know that whatever was inside that .htaccess file is invalid. If we set up the .htaccess files in those test dirs such that one of them has the whole feature set, the next one doesn't have Options, the next one doesn't have mod_rewrite, the final one has an empty .htaccess file, etc. then depending on which ones cause 500 errors we can tell what needs to be fixed on the server in order to get the feature working. And we'll know that we're not going to take down the user's gallery2 install if the webserver doesn't support .htaccess at all.

AdminRewrite.tpl

• I'd rename some of the errors to be a little more descriptive (nofile => missing, blind => cantRead, protected => cantWrite).
• 52: "hte" => "the"
• 67,73: the <pre> tag should be indented to column 4
• 70: Gallery2 -> Gallery

Htaccess.tpl

• Missing standard boilerplate template comment
• Is the Options line required for this feature to work? I'm wondering because I've seen servers where trying to change options will cause a 500 error.

RewriteUrlGenerator.class

• 79: file_exists -> $platform->file_exists()
• 74: operators should go on the line before the break (so move the "+" up from line 75)

RewriteHelper.class

• getTemplateName() doesn't need to return a status code; it cannot generate a bad status. Also, internally to PHP you can always use forward slashes so you could just have it be a one liner:
  

  return dirname(__FILE__) . '/../templates/Htaccess.tpl';
  

  But then it's really not much more than a constant so I'm not sure that it's really necessary to have this method at all. I'll go on to explain how you can get rid of this method entirely.

• in getTemplate() we should make Htaccess.tpl a real Smarty template instead of just a data file (like all the other .tpl files):
  

  $Htaccess = array();
  $Htaccess['baseFile'] = $baseFile;
  $template = new GalleryTemplate();
  $template->setVariable('Htaccess', $Htaccess);
  list ($ret, $htaccess) = $template->fetch('gallery:modules/rewrite/templates/Htaccess.tpl', 'modules_rewrite');
  if ($ret->isError()) {
    return array($ret->wrap(__FILE__, __LINE__), null);
  }
  

  Then in Htaccess.tpl, change "{basefile}" to "{$Htaccess.baseFile}" and that should be all. The advantage is that now if we choose we can use Smarty looping constructs, localization, etc all inside the template file. It will allow people to create their own local version of the template, etc. Note that the template system uses the relative path from the gallery2 directory, so you don't need to use RewriteHelper::getTemplateName() here. If you want to optimize this, you can store the results of the template into a static variable so that subsequent calls have no overhead.

• all uses of file_get_contents() should be replaced by $platform->file() since file_get_contents requires PHP 4.3+
• 64: "Cant" -> "Can\'t"
• 83: substr(..., -24) is brittle. If we change anything, it'll break. Better to use dirname(dirname(dirname(__FILE__))), etc.
• getFileName() is generic, how about getHtaccessPath() instead?
• in checkFile(), see my comment about error names above. There's no need to use short names here. The longer the name, the more meaning it imparts and the less it leaves to imagination.
• checkFile(): $orig is never used
• checkFile(): I think a better API would be to return a true/false value to indicate whether the file is good or not, then return an additional value to indicate what is wrong with the file if you return false. This would simplify your check in writeFile() which right now has to check for the absence of 4 possible different errors. If you add a new error type to checkFile, that code will break. But if you use true/false then that code can just check the boolean.
• writeFile(): $original is unused
• writeFile(): should also return a success/failure boolean like I suggest for checkFile() to simplify error checking for its consumers.

AdminRewrite.inc:

• 53,107: use isset() instead of !is_null() (positive logic is easier to read than negative)
• 86: urlGenerator isn't used
• the template container variable used in AdminRewriteView should be called $AdminRewrite. The canonical form is:
  

  $AdminRewrite = array();
  $AdminRewrite['file'] = $file;
  ...
  $template->setVariable('AdminRewrite', $AdminRewrite);
  

module.inc

• 35: "Enabeles" -> "Enables"
• 54: use isset() instead of !is_null() (positive logic is easier to read than negative)
• 88: We need to come up with a better way for you to to do this. It's next on my list after I finish a couple more code reviews. This is fine for now, though.
• 103: this line should be indented to 28 columns
• I don't think that this module can be auto configured. See my general comment above on "test strategies" for a discussion of why.

[/]

[/]

[/]

[/]

[/]

[/]

[/]

Not quite. It creates URLs like this:

http://your.server/gallery2/main.php/v/album1/album2/foo.jpg

So main.php is passed "/v/album1/album2/foo.jpg" and it uses the first path element (/v) to determine what action it should take. This namespace can never conflict with anything inside the gallery2 directory because main.php can't be a directory AND a file.

In the solution that Pelle was proposing, we'd have a url like this:

http://your.server/gallery2/album1/album2/foo.jpg

Which could cause a conflict...

Oh..I misunderstood what you meant. In my inspection of the code I didn't see that it was generating urls with the /v prefix. My mistake!

Replying to myself... Well, its time for an update on the project. I've added ability to customize URL prefixes. You're not bound to 'v' or 'd' anymore. However, I still cant register my UrlGenerator in time which means that the only prefixes that actually works are 'view' and 'download' (other than 'v' and 'd').

What I haven't done yet is tests to see if mod_rewrite works. Nor have I made unit tests for my class and helper functions. The way I see it I'll fix the functionality first, then make tests to see if it works the way I want it to. Right now, it doesn't (you still need to hack the original core class).

This sounds like an exelent idea. Iframes is not valid xhtml though, and to load pages with php you need allow_url_fopen. I guess the best way to do this is letting the user click and discover if it works for themselves, or maybe a combination of the allow_url_fopen and the manual way. With allow_url_fopen I see a potential way to make the module auto configurable. How common is allow_url_fopen? Is it worth the hassle or should I stick with the manual click method?

Hmm, that is strange. I've just updated to the latest cvs version and I dont get those warnings. With what data are you trying to save?

Edit: Think I've found it. You tried to save with an empty field, right?

This is looking really good. It's very close to the point where you can commit it. You're still missing the unit tests, but as you said in your last post, you're going to do those later. Your code style is excellent; this module will blend right in with the rest of the code base. I'm thinking that when this module is in, it might make sense to ditch the PathInfo short url style altogether. This will be way more powerful and as long as we have this as an option we really don't need PathInfo!

Because of my recent API changes, I wasn't able to test the module. I hope to be able to test it next time around.

module.inc

• I've refactored the way that we create the url generator so that now you can override the url generator properly. You need to make a few changes to bring things up to date:
  
  • Remove performFactoryRegistrations from your callback list in the constructor
  • Update your required core api to 2.1
  • Get rid of your performFactoryRegistrations method and put the following code into your upgrade() method:
    

        $ret = GalleryCoreApi::unregisterFactoryImplementationsByModuleId($this->getId()); 
        if ($ret->isError()) { 
            return $ret->wrap(__FILE__, __LINE__); 
        } 
    
        $ret = GalleryCoreApi::registerFactoryImplementation(
            'GalleryUrlGenerator', 'RewriteUrlGenerator', 'RewriteUrlGenerator',
            'modules/rewrite/classes/RewriteUrlGenerator.class', 'rewrite', null, 3);
        if ($ret->isError()) { 
            return $ret->wrap(__FILE__, __LINE__); 
        }
    

    then bump your module's version and once you upgrade, Gallery should be using your module's url generator instead of the one in core. I haven't had a chance to test this yet so you're going to be on the bleeding edge here (sorry!). Now the trick is to find a place where you can load your prefix map. This is tricky because the only methods that are going to get called are in RewriteUrlGenerator and none of those expect to send back GalleryStatus return codes. So for now, I'd move the code that loads the prefix map into RewriteUrlGenerator::init(), and if you get an error status back just swallow it and leave your prefix map empty and limp along without it. You could set an error bit and then just delegate all calls to GalleryUrlGenerator if there's an error init(), which would basically just take your rewrite class out of the equation.

RewriteUrlGenerator.class

• The API for this class has changed, but it's simple. The constructor went away, and there's a new method called "init" that does everything the constructor used to. So change your constructor to:
  

      function init($baseFile, $relativeG2Path=null, $embedSessionString=null) { 
          parent::init($baseFile, $relativeG2Path, $embedSessionString); 
      } 

• Now that you've got the url generator getting loaded in the right place, registerCustomViewPrefix() can go away or get rolled into init()

AdminRewrite.inc

• as long as we're storing non-normalized data, why not store a serialized hash map? eg: $ret = GalleryCoreApi::setPluginParameter('module', 'rewrite', 'prefixMap', serialize($form['prefix']));
  Then in RewriteHelper::getPrefixMap:

   list ($ret, $map) = GalleryCoreApi::getPluginParameter('module', 'rewrite', 'prefixMap');
  // check $ret
  $map = unserialize($map);
  

  Less code, one less database lookup, unserialize is very fast, you don't have to worry about views/prefixes becoming de-synced.

• Since you're only returning one error, it doesn't make sense to me to translate the nice descriptive constants into strings and then check them in the template as the strings. I think a better approach is to just keep them as constants all through the code. This means that in AdminRewrite you'd say:
  $form['error']['code'] = $error; then in AdminRewrite.tpl you'd have:

  {if isset($form.error.code)}
  {if ($form.error.code == REWRITE_STATUS_MISSING)}
  <div class="giError"> 
    {g->text text="The .htaccess file does not exist."} 
  </div> 
  {/if}
  ...
  {/if}
  

  . It's a little different from the normal pattern, but it's pretty readable.

RewriteHelper.class

• The constants look great. I'd change them to be numeric though instead of string values (eg define('REWRITE_STATUS_MISSING', 1); etc)
• 151: check the value of $ret
• 155: the && should go on the line above
• See my comment in AdminRewrite about simplifying getPrefixMap()

AdminRewrite.tpl

• Strings like "Or let Gallery do the job for you. All you need..." are difficult to localize since sentence fragments depend on the context which isn't always obvious to the localizer. It'd be better to say something like "Gallery can do the job for you. All you need..."

[/]

[/]

[/]

[/]

[/]

[/]

maybe PathInfo could be a separate module instead? then you can choose which short-url module you want to use...

I agree that thos urls are longer than they were before. But they're definitely more descriptive and we won't have conflicts this way.

If we preserve file extensions, then they have to go into the URL or if we have foo.jpg and foo.avi in the same album then /album/foo won't be enough for us to tell the difference. If we strip away the file extension, then we have to also do it to the file name *on disk* because currently we map the name of the file directly to the url.

The only alternate approach I can think of is to create a new field for every item which is a unique name that has no extension. So if you add "foo.jpg" to an album then its unique name would be "foo" and we could use that in the url. Then if you add "foo.avi" to the same album it might wind up with a unique name of "foo_001" or "foo-avi" or something. That would not be so hard to do, but it would be one more field that people would have to edit when they modify their items.

What do you think?

But the filename is already unique and editable, isn't it?
So what about

- strip extension from original filename
- if filename exists, add "-1", "-2", etc.

/gallery2/v/myalbum/foo
/gallery2/v/myalbum/foo-1
/gallery2/v/myalbum/foo-2
...

The problem with id's is, you can't delete and re-upload an image without changing its URL...

This is what I meant - I was referring the "filename" field on the G2 admin form. So maybe "store files with their original names, *copy* filenames to the "filename" field (maybe rename it to "url name") and strip the extension" would be a solution.

That's a good idea. I've changed it so that we only apply the .html to non-albums.

Ryan, your suggestion will still have the same problem with conflicts if they have a root album name that coincides with some other directory in their document root. Ie, if they have a G2 album named "AlbumX" and they happen to have a directory called "AlbumX" then there's nothing that you can do in.htaccess to divine which link they really want.

But in general, I think that the rewrite module that Pelle is developing should be able to do this. After he gets it committed, we could tune it so that you can specify exactly where you want the .htaccess file to live and let you specify the base url for G2. This would allow you to install your G2 into http://your.server/gallery2/ but still be able to access it as http://your.server/, provided you have support for .htaccess

This sounds like a great idea. In fact I've been thinking somewhat along those lines for new features, but first I want to finnish up the basic functionality.

Edit: I'd like to extend it to beond viewing core:ShowItem as in your example. For instance these virtual URLs:
http://yoursite.com/gallery/album/image1.jpg.html
http://yoursite.com/download/album/image1.jpg
This does not work if you want:
http://yorsite.com/album/imag2.jpg.html
Here you get the namespace problem too.

I'm in the process of reviewing this now. I have some preliminary notes for you:
General

• Apache mod_rewrite test fails, but gives no indication of what is wrong.
• The "Write File" button shouldn't appear if we can't write to the .htaccess file
• Move the "Can not find Gallery data" message up to the Testing block so that it's clear that this is an explanation of why the "Gallery .htaccess" test failed
• Clicking "test setup" gives me:
  

  Notice: Undefined index: write in /usr/home/bharat/public_html/gallery2/modules/rewrite/SetupRewrite.inc on line 53
  

  A controller test would have prevented this from slipping through.

The main problem I'm hitting (before even getting into the review) is that it doesn't work for me. The Apache mod_rewrite test generates a url like this:

http://me.com/g2/modules/rewrite/test/mod_rewrite/?match=/test/

But, when that gets rewritten, it doesn't have $_SERVER['PATH_INFO'] set, so it would appear that PathInfo doesn't work properly. Except that PathInfo *does* work properly for me if I enable short urls via the Site Admin. So I'm thinking that something about how mod_rewrite regenerates the URL is causing Apache2 not to pick it up correctly. I'm testing this with Apache/2.0.52 (Debian GNU/Linux) PHP/4.3.10-2.

I'm going to go read the mod_rewrite manual and see what I can come up with. If we can't get it working, then perhaps an alternative approach would be to provide a path= argument to core:ShowItem so that you can provide the path there instead of having to use PathInfo. I think that would be more reliable in general. What do you think?[/]